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

chore(er): capture exception API #9714

Closed

Conversation

P403n1x87
Copy link
Contributor

@P403n1x87 P403n1x87 commented Jul 4, 2024

We add a new programmatic API for Exception Replay to allow users to capture caught exceptions explicitly.

Checklist

  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

We add a new programmatic API for Exception Replay to allow users to
capture caught exceptions explicitly.
@P403n1x87 P403n1x87 self-assigned this Jul 4, 2024
@P403n1x87 P403n1x87 added changelog/no-changelog A changelog entry is not required for this PR. Dynamic Instrumentation Dynamic Instrumentation/Live Debugger labels Jul 4, 2024
@EWachnowezki
Copy link

just checked the API method name and looks good to me :)

@P403n1x87 P403n1x87 marked this pull request as ready for review July 8, 2024 11:15
@P403n1x87 P403n1x87 requested a review from a team as a code owner July 8, 2024 11:15
@P403n1x87 P403n1x87 enabled auto-merge (squash) July 8, 2024 11:15
Copy link
Contributor

@shatzi shatzi left a comment

Choose a reason for hiding this comment

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

This is awesome. I think there are two things missing:

  1. we need documentation both on change log here and on exception replay docs to expose this feature
  2. figure out if capture_exception() works on non service entry spans with Error Tracking. if that not the case - make sure we document this as known limitation or have a follow up PR to find a solution


try:
span = tracer.current_span()
if span is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

how this works when

  1. a user call capture_exception() multiple times? will the first one/last one win? do we capture all snapshots multiple times?
  2. how this works if the exception also propagate to the span it self? is that same as call "capture_exception" twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiple calls would cause the data to be emitted multiple times. We could probably guard against this by checking if the span has error already set, and skip.

how this works if the exception also propagate to the span

I'm not quite sure I understand this. With this call we are setting an error on the span. However, we don't expect this to be used when the exceptions are not handled. This will only work in an exception handler, as anywhere else in the code there won't be an exception to attach to a span. If the exception is re-raised within an exception block however, then the tracer will trigger the capturing of debug data a second time.

Copy link
Contributor

Choose a reason for hiding this comment

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

You got it. Therefore, I think this logic is little broken and should emit debug info only the last/first error that was assigned to the span.

from ddtrace import tracer

try:
span = tracer.current_span()
Copy link
Contributor

Choose a reason for hiding this comment

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

if trace._current_span() is not the service entry, will this still work?

I heard there is a tag ET support to index that span error if it not the service entry. maybe this should also set that tag. However, I can't seem to find it on their docs. so maybe this should add tags to service entry span (or both entry and current) or simple abort for non-entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if trace._current_span() is not the service entry, will this still work?

This will put debug info on the current span, regardless of what the span is. So any limitations are probably not due to "us" I'd expect

Copy link
Contributor

@shatzi shatzi Jul 10, 2024

Choose a reason for hiding this comment

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

yup. this is broken as non entry spans are not get indexed and there is no way for the user to query on that and it would not appear in Error tracking. so not sure why we allow the user to do it in the first place.

cc @EWachnowezki

@P403n1x87
Copy link
Contributor Author

Closing in favour of #10430.

@P403n1x87 P403n1x87 closed this Aug 28, 2024
auto-merge was automatically disabled August 28, 2024 15:06

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR. Dynamic Instrumentation Dynamic Instrumentation/Live Debugger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants