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 common gen AI utils into opentelemetry-instrumentation #3188

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Jan 15, 2025

Description

Part of #3191

(Copied out of cohere PR #3081)

Type of change

Refactor

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added tests

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added

@aabmass
Copy link
Member Author

aabmass commented Jan 15, 2025

Lint and tests for OpenAI are failing because we test against oldest published supported dependencies

# packages that are released individually should provide a test-requirements.txt with the lowest version of OTel API
# and SDK supported to test we are honoring it
openai-0: -r {toxinidir}/instrumentation-genai/opentelemetry-instrumentation-openai-v2/test-requirements-0.txt
opentelemetry-api==1.28 # when updating, also update in pyproject.toml
opentelemetry-sdk==1.28 # when updating, also update in pyproject.toml
opentelemetry-semantic-conventions==0.49b0 # when updating, also update in pyproject.toml

To move this PR forward we either need to

  1. Disable "oldest supported" tests py3x-test-instrumentation-openai-v2-0
  2. Wait until next release of opentelemetry-instrumentation is published to update OpenAI code

@aabmass
Copy link
Member Author

aabmass commented Jan 15, 2025

2. Wait until next release of opentelemetry-instrumentation is published to update OpenAI code

I decided to do this, I've split the openai changes for now. I'll make a tracking bug to update the code once this PR is merged.

@aabmass aabmass force-pushed the genai-utils branch 2 times, most recently from 4175da2 to dcd3e4c Compare January 15, 2025 23:15
@aabmass aabmass marked this pull request as ready for review January 15, 2025 23:23
@aabmass aabmass requested a review from a team as a code owner January 15, 2025 23:23
def get_span_name(span_attributes: Mapping[str, AttributeValue]) -> str:
name = span_attributes.get(GenAIAttributes.GEN_AI_OPERATION_NAME, "")
model = span_attributes.get(GenAIAttributes.GEN_AI_REQUEST_MODEL, "")
return f"{name} {model}"
Copy link
Contributor

Choose a reason for hiding this comment

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

model name is not always available, we should return name in this case

return f"{name} {model}"


def handle_span_exception(span: Span, error: Exception) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe

Suggested change
def handle_span_exception(span: Span, error: Exception) -> None:
def end_span_with_exception(span: Span, error: Exception) -> None:

?

we're not handling anything, right?

BTW - there is nothing gen-ai specific here, should we move it to common instrumentation helpers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh I'm kind of just mechanically copying from openai-v2:

def handle_span_exception(span, error):
span.set_status(Status(StatusCode.ERROR, str(error)))
if span.is_recording():
span.set_attribute(
ErrorAttributes.ERROR_TYPE, type(error).__qualname__
)
span.end()

Actually, the API/SDK has a mechanism here when used in start_as_current_span ctx manager. That would also call span.record_exception() which I believe add a span event with the stacktrace.

cc @lzchen @alizenhom do you know why OpenAI doesn't just use the existing mechanism?

@aabmass
Copy link
Member Author

aabmass commented Jan 17, 2025

Putting this back in draft for now, see the comments on linked issue #3191 (comment)

@aabmass aabmass marked this pull request as draft January 17, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants