Add some sem conv attributes to the stable vertex AI and gen AI instrumentations #4011
Conversation
| if not content.parts: | ||
| return | ||
| # System instruction is required to be text. An error will be returned by the API if it isn't. | ||
| system_instruction = " ".join( | ||
| part.text for part in content.parts if part.text | ||
| ) |
There was a problem hiding this comment.
Could we just model_dump the full provided system instructions Content object without converting into a string? It's true it should be text, but a user might pass non-text and need to debug.
There was a problem hiding this comment.
if we use model_dump the output is nested and kind of ugly, also it'll look weird if the user just passed a string or a list of strings to system_instructions (which the gen AI SDK lets people do). The error will get surfaced to the user in the error.type attribute I believe, and also all their calls will be failing so I think they will be able to figure it out without this.
As an aside I'd like to add exclude_unset=True to the existing model_dumps that we do so we aren't dumping a long list of unset fields into the telemetry..
There was a problem hiding this comment.
Actually I'm tempted to just do that in this PR... Are you on board with that ?
There was a problem hiding this comment.
if we use model_dump the output is nested and kind of ugly, also it'll look weird if the user just passed a string or a list of strings to
system_instructions(which the gen AI SDK lets people do).
I think they will have the opposite problem with the PR's current code, if they pass Parts right now it will get converted to a joined string. It's true the error gets should surface but imo it's confusing if the error conflicts with the captured params. It's up to you on this one, I think it's minutiae either way.
As an aside I'd like to add
exclude_unset=Trueto the existingmodel_dumpsthat we do so we aren't dumping a long list of unset fields into the telemetry..
SGTM
instrumentation-genai/opentelemetry-instrumentation-google-genai/CHANGELOG.md
Outdated
Show resolved
Hide resolved
instrumentation-genai/opentelemetry-instrumentation-vertexai/CHANGELOG.md
Outdated
Show resolved
Hide resolved
...trumentation-google-genai/src/opentelemetry/instrumentation/google_genai/generate_content.py
Outdated
Show resolved
Hide resolved
7 participants
Description
Add some sem conv attributes to the stable vertex AI and gen AI instrumentations that were missing.
Fix a bug in how system instructions were recorded in the gen ai system event in the stable gen AI instrumentation.
Fixes #3934 and #3979
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit tests
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.