-
Notifications
You must be signed in to change notification settings - Fork 782
Update google genai instrumentation to work with latest semantic convention, allow for uploading content. #3772
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?
Conversation
...trumentation-google-genai/src/opentelemetry/instrumentation/google_genai/generate_content.py
Outdated
Show resolved
Hide resolved
ContentCapturingMode.SPAN_ONLY, | ||
ContentCapturingMode.SPAN_AND_EVENT, | ||
]: | ||
span = trace.get_current_span() | ||
span.set_attributes(completion_details_attributes) | ||
if self._content_recording_enabled in [ | ||
ContentCapturingMode.EVENT_ONLY, | ||
ContentCapturingMode.SPAN_AND_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.
@DylanRussell should we still do this if there is an upload hook? One practical concern for GCP logging exporter is that if you include the JSON in the log directly and it exceeds the size limit (256kB) the whole log gets dropped. Since the upload hook works on the same log, that would mean everything gets dropped.
Alternatively, maybe we could emit two separate logs to avoid this problem
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.
The user can both configure whether they want the upload and whether they want the content in the log right ? So maybe lets just leave it to the user and recommend one way or the other ? Probably should discuss it more internally..
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.
Ah thanks I think you're right from semconv
The hook SHOULD operate independently of the opt-in flags that control capturing of
gen_ai.system_instructions
,gen_ai.input.messages
, andgen_ai.output.messages
.
I kind of read this the opposite way at first but it makes sense.
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.
So the code should always call the hook
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.
So, IIUC, the hook operates independently, is called always (potentially a noop), potentially stamps the attributes with refs, and if span/event logging is enabled, that then is logged, yes?
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.
@DylanRussell can you share the code you did since you had a good look at the specs?
I think we would always create the event and span as well and pass them to the hook. But just not add the content directly to them depending on the environment variable.
...trumentation-google-genai/src/opentelemetry/instrumentation/google_genai/generate_content.py
Outdated
Show resolved
Hide resolved
...trumentation-google-genai/src/opentelemetry/instrumentation/google_genai/generate_content.py
Outdated
Show resolved
Hide resolved
...trumentation-google-genai/src/opentelemetry/instrumentation/google_genai/generate_content.py
Outdated
Show resolved
Hide resolved
ContentCapturingMode.SPAN_ONLY, | ||
ContentCapturingMode.SPAN_AND_EVENT, | ||
]: | ||
span = trace.get_current_span() | ||
span.set_attributes(completion_details_attributes) | ||
if self._content_recording_enabled in [ | ||
ContentCapturingMode.EVENT_ONLY, | ||
ContentCapturingMode.SPAN_AND_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.
The user can both configure whether they want the upload and whether they want the content in the log right ? So maybe lets just leave it to the user and recommend one way or the other ? Probably should discuss it more internally..
e76d888
to
d0d6f1a
Compare
Co-authored-by: Aaron Abbott <[email protected]>
d0d6f1a
to
ee911f5
Compare
5d0572a
to
8583327
Compare
Description
Accomodation of semantic conventions changes made in open-telemetry/semantic-conventions#2179.
Also, when logging the completion details, the upload hook is called, so if user has configured
OTEL_INSTRUMENTATION_GENAI_UPLOAD_BASE_PATH
andOTEL_INSTRUMENTATION_GENAI_UPLOAD_HOOK
env vars, completion details will also be logged as refs (if appropriate capture content env var is set).Type of change
How Has This Been Tested?
Unit tests, manual tests.
Does This PR Require a Core Repo Change?
No?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.