- 
                Notifications
    
You must be signed in to change notification settings  - Fork 803
 
OpenAI v2 onboard onto semantic conventions 1.37.0: chat history and other breaking changes #3715
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
base: main
Are you sure you want to change the base?
OpenAI v2 onboard onto semantic conventions 1.37.0: chat history and other breaking changes #3715
Conversation
| 
           As a common package we have https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/util/opentelemetry-util-genai  | 
    
| 
           thanks a lot, @xrmx!  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions, but overall it looks good for this iteration. Thanks for doing it, we can add some notes to follow up on these items in the following pr.
| # `gen_ai_latest_experimental` is not set in the | ||
| # `OTEL_SEMCONV_STABILITY_OPT_IN` environemnt variable. | ||
| # - everything else - don't record content on any signal | ||
| OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=span | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optionally - introduce a new OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT_MODE variable, which will include a list of supported modes - i.e.
spaneventspan,event- sometimes preferred for development/teasting.
Reusing OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT seems fine too.
if we decide to support span,event, it will require either introducing ContentCapturingMode.SPAN_AND_EVENT or supporting a list of capturing modes. May be helper methods like is_capturing_span() and is_capturing_event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now we have two config items in Spring AI prototype:
OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT- (for compatibility) decide whether we capture the chat messages.false- (by default) don't capture any chat messagestrue- capture all chat messages, specific behaviors depend on additional configuration
OTEL_INSTRUMENTATION_GENAI_MESSAGE_CONTENT_CAPTURE_STRATEGY- decide how we capture the chat messages.span-attributes- (by default) capturing them as span attributesevent- capturing them as a eventevent, span-attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cirilla-zmh , thanks for sharing!
As a long-time java developer - can't resist the comment VERY_JAVA_STYLE_VERBOSE_EVNIRONMENT_VARIABLE_NAMING 🤣
But seriously
- 👍 
span-attributesfor clarity. - 👍 a list of modes "
event, span-attributes, it is quite popular for testing. - 👎 different word ordering in 
CAPTURE_MESSAGE_CONTENTandMESSAGE_CONTENT_CAPTURE_STRATEGYin env naming. I would 👍 to reuseOTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENTenv var. 
cc: @lmolkova
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a long-time java developer - can't resist the comment VERY_JAVA_STYLE_VERBOSE_EVNIRONMENT_VARIABLE_NAMING 🤣
Ahh, that's it...🤣 But it comes from the requirement of multi-language compatibility and isolation, it's more clear when transported to a property key: otel.instrumentation.genai.capture_messages_content.
For compatibility, I tend to add another config key to control the behavior of the messages capturing - the naming could be discussed again.
| return traced_method | ||
| 
               | 
          ||
| 
               | 
          ||
| def _record_metrics( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we move _record_metrics() and set_response_attributes to the utils.py
| "", | ||
| tracer_provider, | ||
| schema_url=Schemas.V1_28_0.value, | ||
| schema_url="https://opentelemetry.io/schemas/1.37.0", # TODO: Schemas.V1_37_0.value, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably need a telemetry test for the attributes in this telemetry.
          
 @xrmx , I would propose we merge this one before the common library is ready. It will provide testing needed to switch to the new library. Also, please review the PRs which are pre-requirement to switching to the util library.  | 
    
Implements new semconv 1.37.0 behind
OTEL_SEMCONV_STABILITY_OPT_IN = gen_ai_latest_experimentalopt-in.Depends on #3709 (common code for
OTEL_SEMCONV_STABILITY_OPT_IN) and open-telemetry/opentelemetry-python#4731 (new semconv package).TODOs:
ContentCapturingMode(we should put it into https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/util/opentelemetry-util-genai, see @xrmx comment below)