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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac87a1f
fix(watsonx): use with statement to automatically handle exception
jinsongo Oct 18, 2024
74eaaa0
fix(watsonx): explicitly call span.__exit__ for exception record
jinsongo Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...s/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import logging
import os
import sys
import types
import time
from typing import Collection, Optional
Expand Down Expand Up @@ -480,6 +481,9 @@ def _wrap(
if exception_counter:
exception_counter.add(1, attributes=attributes)

exc_type, exc_val, exc_tb = sys.exc_info()
span.__exit__(exc_type, exc_val, exc_tb)

raise e

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 •
edited
Loading

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.

span.__exit__ implementation:
https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L986-L1008
span.__exit__ of parent class:
https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/span.py#L186-L194

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

Expand Down