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

fix(watsonx): set span with exception information #2172

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jinsongo
Copy link
Contributor

@jinsongo jinsongo commented Oct 18, 2024

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Use with statement to catch the exception and automatically mark the span as being in an error state, then if an exception occurs, the span can be properly ended and contain information about the exception.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. python Pull requests that update Python code labels Oct 18, 2024
@jinsongo
Copy link
Contributor Author

The fix is to resolve the problem of no trace for the erroneous call that's caused by incorrect watsonx API key. cc @gyliu513


if "generate" in name:
Copy link
Member

Choose a reason for hiding this comment

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

I think this won't work well with the generator response. You need to manually close the span on all cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will investigate and update with more details.

Copy link
Contributor Author

@jinsongo jinsongo Oct 19, 2024

Choose a reason for hiding this comment

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

Explicitly call span.__exit__ for exception, just like using with statement to do it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nirga please review again. span.__exit__ is the default operation to record exception information and call span.end(), and no impact to generator response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, bedrock instrumentation use with statement and no 'Generator' problem, so the exception can be handled by span.__exit__ automatically when exception raised from line 166 response = fn(*args, **kwargs)
https://github.com/traceloop/openllmetry/blob/main/packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.py#L163-L171

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes @jinsongo but you're setting the exception yourself anyway so you don't really need this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to set the exception information into the span by simply calling __exit__.
As you know, without the fix, I cannot see any span sent after the exception raised.

Copy link
Member

Choose a reason for hiding this comment

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

@jinsongo why? You didn't call __enter__ so I don't think it's preferred on explicitly logging the exception on the span

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Oct 19, 2024
@jinsongo jinsongo changed the title fix(watsonx): use with statement to automatically handle exception fix(watsonx): set span with exception information Oct 19, 2024
@jinsongo jinsongo marked this pull request as draft November 4, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants