GenAI Utils | Add more SemConv Attributes #3862
Conversation
| _maybe_set_span_messages( | ||
| span, invocation.input_messages, invocation.output_messages | ||
| ) | ||
| _apply_request_attributes(span, invocation) |
There was a problem hiding this comment.
not blocking for this PR, but we should eventually allow to set attributes at start time. I noticed we don't properly mark which attributes should be provided at the start time, fixing it in the open-telemetry/semantic-conventions#2994
| else: | ||
| finish_reasons = None | ||
|
|
||
| if finish_reasons: |
There was a problem hiding this comment.
I believe if finish_reasons is not available, we should set error - open-telemetry/semantic-conventions#2919 (comment)
There was a problem hiding this comment.
hmm what would you set error to in that case ? I feel like instrumentations should set error explicitly (or call into a util that sets it explicitly) rather than try to infer it like that
There was a problem hiding this comment.
I think Liudmila means literally setting the reason to error finish_reason = FinishReasons.ERROR. Not talking about the error attribute itself
util/opentelemetry-util-genai/src/opentelemetry/util/genai/types.py
Outdated
Show resolved
Hide resolved
util/opentelemetry-util-genai/src/opentelemetry/util/genai/span_utils.py
Outdated
Show resolved
Hide resolved
| if invocation.finish_reasons is not None: | ||
| finish_reasons = invocation.finish_reasons | ||
| elif invocation.output_messages: | ||
| finish_reasons = [ |
There was a problem hiding this comment.
wonder if this should be a sorted set instead of a list ? or at least converted to a set and then converted back to a sorted list to get rid of duplicates
There was a problem hiding this comment.
Finish reasons are sorted and unique now. some unit tests for this added as well.
| def _new_str_any_dict() -> dict[str, Any]: | ||
| return {} | ||
|
|
||
|
|
There was a problem hiding this comment.
So my reading of https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/ is that "recommended" attributes SHOULD be included by default.. So for the attributes that correspond to these fields that are "recommended" I think we should be defaulting these to empty strings and 0 values instead of None (which means they will not be added).. Thoughts @aabmass @lmolkova ?
5 participants
Description
Add missing SemConv attributes to spans for LLMInvocation calls, clean up some repeated code in the unit tests for genai utils.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Updated unit tests to check for additional attributes
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.