Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to configure pg error_handler #3303

Conversation

alexevanczuk
Copy link
Contributor

@alexevanczuk alexevanczuk commented Dec 1, 2023

What does this PR do?

This PR allows the client to configure an error_handler for the pg integration, which does not currently support an error_handler as many of the other integrations.

Motivation:

Application level rescuing

Take the following ruby code snippet:

begin
  insert_non_unique_record
rescue ActiveRecord::RecordNotUnique => e
  # We catch this error and handle it gracefully
end

In this example, the pg integration reports an PG::UniqueViolation error to datadog. This error is pure noise, because our application code catches the exception generated by the postgres ruby gem. Since we're rescuing this, it conceptually makes sense to also not report the postgres error.

Application level handling of all errors

Consider this code snippet:

insert_non_unique_record

Even though this is not rescued as above, the PG::UniqueViolation will propagate through the stack and eventually be caught by the overall Datadog middleware for web or async transactions and be reported to Datadog. To avoid double reporting of errors, we'd like to ignore these.

Console level mistakes

Consider I'm in console and I make an error in my postgres query. This will be reported to datadog. I might want to ignore errors from console or just ignore all errors unless other parts of the stack rescue errors in console and report or log them.

Additional Notes

Let me know if there's other best practices for implementing this change.

For what it's worth – I also tried to ignore this through some other various hacks.

I tried this:

Datadog::Tracing.before_flush do |trace|
  trace.spans.each do |span|
    # See note above about service naming.
    next if span.get_tag('service') != 'pg'
    span.clear_tag('error')
  end

  trace
end

But this did not work. It looked like it did not actually clear the error span tag or the error status from the trace.

I'd love some other way to be able to more forcibly override any integration to remove errors from certain spans where they do not apply, but it does not appear there is a public API to do this yet.

How to test the change?

I followed patterns in the faraday/middleware_spec.rb to add tests to cover this.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@alexevanczuk alexevanczuk requested review from a team as code owners December 1, 2023 13:39
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Dec 1, 2023
@@ -81,10 +81,12 @@ def sync_exec_prepared(statement_name, params, *args, &block)

def trace(name, sql: nil, statement_name: nil, block: nil)
service = Datadog.configuration_for(self, :service_name) || datadog_configuration[:service_name]
error_handler = datadog_configuration[:error_handler]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know if there is another standard way to get the error handler here.

@@ -25,6 +25,11 @@ class Settings < Contrib::Configuration::Settings
o.default false
end

option :error_handler do |o|
o.type :proc
o.default_proc(&Tracing::SpanOperation::Events::DEFAULT_ON_ERROR)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best I can tell this is the literal constant that represented the existing default behavior, so I wanted to reference it explicitly here to maintain existing behavior.

@alexevanczuk alexevanczuk force-pushed the ae-add-ability-to-configure-pg-error-handler branch from fd65189 to 243664c Compare December 1, 2023 13:52
Copy link
Contributor

@urseberry urseberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor grammar nit, otherwise approved.

docs/GettingStarted.md Show resolved Hide resolved
@alexevanczuk
Copy link
Contributor Author

Merged your suggestion, thanks @urseberry ! Let me know if there's anything else I can help with to get this merged and released. Thanks for your prompt review!

@alexevanczuk alexevanczuk force-pushed the ae-add-ability-to-configure-pg-error-handler branch from a9843f3 to 243664c Compare December 1, 2023 22:03
@alexevanczuk
Copy link
Contributor Author

Hey @urseberry wanted to bump this one! Let me know if there's anything else ya need 🙏🏼

@urseberry
Copy link
Contributor

urseberry commented Dec 4, 2023

Hey @urseberry wanted to bump this one! Let me know if there's anything else ya need 🙏🏼

Thanks, Alex! I already approved on behalf of Documentation, but you also need an engineer to review and approve. It's in their queue and I'm sure someone will get to your PR soon.

@marcotc marcotc merged commit f3358c0 into DataDog:master Dec 6, 2023
68 checks passed
@github-actions github-actions bot added this to the 1.18.0 milestone Dec 6, 2023
@alexevanczuk
Copy link
Contributor Author

Thanks @marcotc for the merge! No rush, but just curious to have a sense of release cadence!

@alexevanczuk alexevanczuk deleted the ae-add-ability-to-configure-pg-error-handler branch December 7, 2023 02:49
@TonyCTHsu TonyCTHsu mentioned this pull request Dec 7, 2023
@ekump
Copy link
Contributor

ekump commented Dec 7, 2023

@alexevanczuk We released 1.18.0 today and this has been included. Thank you for your contribution!

@alexevanczuk
Copy link
Contributor Author

Thanks so much @ekump !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants