Add id and finish_reason to OpenTelemetry instrumentation (closes #886) #1882
Conversation
| if response.vendor_id is not None: | ||
| new_attributes['gen_ai.response.id'] = response.vendor_id | ||
| if response.vendor_details is not None: | ||
| new_attributes['gen_ai.response.finish_reasons'] = json.dumps(response.vendor_details) |
There was a problem hiding this comment.
vendor_details can contain a lot more than just finish reasons, so we should just use the finish_reason key.
There was a problem hiding this comment.
I also notice that https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-events/ requires finish_reasons to be a list, and it lists some specific known values we should use:
- content_filter
- error
- length
- stop
- tool_calls
There was a problem hiding this comment.
It seems reasonable to add a new finish_reason field to ModelResponse that has those 5 values as Literal and also allows any string. We should then update the finish_reason code in the Google and Gemini models to try to map to those values if possible.
@Deriverx2 Would you be up for implementing that?
|
Could you check it and provide whether this is ok for mapping in gemini, and provide some context on how to make changes in google model. @DouweM |
| 'OTHER': 'error', | ||
| 'FINISH_REASON_UNSPECIFIED': 'error', # unspecified is still a model stop reason | ||
| 'IMAGE_SAFETY': 'content_filter', | ||
| None: None, |
There was a problem hiding this comment.
I think we can drop this line and it'll still work as expected.
| usage, | ||
| vendor_id=vendor_id, | ||
| vendor_details=vendor_details, | ||
| parts, response.get('model_version', self._model_name), usage, finish_reason |
There was a problem hiding this comment.
It looks like we accidentally dropped the vendor_id!
There was a problem hiding this comment.
Also for backward compatibility reasons, let's keep the untouched finish_reason on vendor_detail as well.
| 'OTHER': 'error', | ||
| 'FINISH_REASON_UNSPECIFIED': 'error', | ||
| 'IMAGE_SAFETY': 'content_filter', | ||
| None: None, |
There was a problem hiding this comment.
Same as up, I think we can drop this
| vendor_details: dict[str, Any] | None = None | ||
| finish_reason = response.candidates[0].finish_reason | ||
| if finish_reason: # pragma: no branch | ||
| vendor_details = {'finish_reason': finish_reason.value} |
There was a problem hiding this comment.
As above, let's keep this one for backward compatibility.
| @@ -275,6 +290,9 @@ async def _generate_content( | |||
| def _process_response(self, response: GenerateContentResponse) -> ModelResponse: | |||
| if not response.candidates or len(response.candidates) != 1: | |||
| raise UnexpectedModelBehavior('Expected exactly one candidate in Gemini response') # pragma: no cover | |||
| finish_reason_key = response.candidates[0].finish_reason or None | |||
There was a problem hiding this comment.
In the original code, we were calling .value on ...[0].finish_reason. Do we need the same here, and if not, why not?
|
@alexmojaki Any chance you want to pick this up? If not I'll close it for inactivity. |
|
No, I can't now |
|
@Deriverx2 I'm closing this PR as it's been a few weeks with no response; if you're still interested in implementing this feel free to reopen it! |
This PR addresses issue #886 by adding support for gen_ai.response.id and gen_ai.response.finish_reasons attributes to the OpenTelemetry instrumented.py implementation, in accordance with the OpenTelemetry semantic conventions.