Skip to content
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

feat: Write OTel GenAI semantic convention client-side metrics for token usage #1545

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bryanatkinson
Copy link
Contributor

Adds support for writing the gen_ai.client.token.usage metric as described by the OpenTelemetry GenAI semantic conventions: https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-metrics/#metric-gen_aiclienttokenusage

I added this directly to the framework instead of in the google-cloud plugin since they seem generally useful. So far I have only added support to the Gemini and Imagen models to see if this approach looks good. If it does, I will carry it through to the other models in the vertex model garden and add the gen_ai.client.operation.duration metric.

Checklist (if applicable):

@bryanatkinson bryanatkinson force-pushed the bryanatkinson-semconv-metrics branch 2 times, most recently from 6795c8d to 1aa5cb4 Compare December 19, 2024 14:49
@bryanatkinson bryanatkinson force-pushed the bryanatkinson-semconv-metrics branch from 1aa5cb4 to 0901d71 Compare December 19, 2024 14:50
@@ -422,7 +422,7 @@ describe('GoogleCloudMetrics', () => {
);
});

it('writes feature label to generate and action metrics when running inside an action', async () => {
it.only('writes feature label to generate and action metrics when running inside an action', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be undone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. Removed.

* conditions we defer the instantiation of the metric to when it is first
* ticked.
*/
class Metric<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of having this be in core - are we positive that this can't live in the plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason it couldn't be done in the plugin, but these semantic conventions are intended so that different tools can understand the telemetry data, so only exporting them when you're using GCP somewhat defeats the purpose.

@bryanatkinson
Copy link
Contributor Author

I've also got a follow-up that includes writing the span attributes and I think cleans things up a little bit: https://github.com/firebase/genkit/compare/bryanatkinson-semconv-metrics...bryanatkinson-semconv-spans?expand=1

I can switch this PR to using that if you're OK with the general approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants