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

genai: add system_fingerprint attribute #1355

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

xrmx
Copy link
Contributor

@xrmx xrmx commented Aug 20, 2024

Changes

Add system_fingerprint attribute to genai to track the value from the OpenAI api response as requested in https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2759/files/6383978266d85eb058fd8030e8da2b83e6d90043#r1710313834

Merge requirement checklist

@xrmx xrmx marked this pull request as ready for review August 20, 2024 08:27
@xrmx xrmx requested review from a team August 20, 2024 08:27
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
Copy link
Member

@drewby drewby left a comment

Choose a reason for hiding this comment

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

I have two questions:

  1. Can we confirm that this is returned from more vendors/models than OpenAI?
  2. How often does system_fingerprint change? If its low cardinality enough, I'd suggest adding this to the metric as well as a good way to correlate between fingerprint changes and degradations/changes in app performance.

@xrmx
Copy link
Contributor Author

xrmx commented Aug 22, 2024

I have two questions:

1. Can we confirm that this is returned from more vendors/models than OpenAI?

I think it's mostly OpenAI specific, not found something similar in Mistral or Anthropic APIs.

2. How often does system_fingerprint change? If its low cardinality enough, I'd suggest adding this to the metric as well as a good way to correlate between fingerprint changes and degradations/changes in app performance.

I don't know

@drewby
Copy link
Member

drewby commented Aug 23, 2024

I have two questions:

1. Can we confirm that this is returned from more vendors/models than OpenAI?

I think it's mostly OpenAI specific, not found something similar in Mistral or Anthropic APIs.

2. How often does system_fingerprint change? If its low cardinality enough, I'd suggest adding this to the metric as well as a good way to correlate between fingerprint changes and degradations/changes in app performance.

I don't know

I have two questions:

1. Can we confirm that this is returned from more vendors/models than OpenAI?

I think it's mostly OpenAI specific, not found something similar in Mistral or Anthropic APIs.

2. How often does system_fingerprint change? If its low cardinality enough, I'd suggest adding this to the metric as well as a good way to correlate between fingerprint changes and degradations/changes in app performance.

I don't know

Can you add this to the metric

I have two questions:

1. Can we confirm that this is returned from more vendors/models than OpenAI?

I think it's mostly OpenAI specific, not found something similar in Mistral or Anthropic APIs.

2. How often does system_fingerprint change? If its low cardinality enough, I'd suggest adding this to the metric as well as a good way to correlate between fingerprint changes and degradations/changes in app performance.

I don't know

I think its worth adding this to the base set for metrics as Conditionally Required, If available : https://github.com/open-telemetry/semantic-conventions/blob/main/model/metrics/gen-ai.yaml

If it exists in Mistral and Anthropic, then it makes sense to keep in in the generic namespace vs a vendor namespace.

@xrmx xrmx marked this pull request as draft August 26, 2024 08:21
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Thanks a lot for following up and adding this @xrmx!

Given that it seems to be OpenAI specific, it'd need to be gen_ai.openai.response.system_fingerprint (or similar) attribute.

But before we can add it, we'd need to add the whole openai specific conventions doc - I created #1370 to track it and it's up for grabs for anyone interested.

@drewby
Copy link
Member

drewby commented Oct 3, 2024

@xrmx OpenAI conventions are now merged. Can you rebase and apply your system_fingerprint attribute to the new openai attributes?

#1385

@xrmx xrmx force-pushed the add-genai-system-fingerprint branch from 961dd63 to 48d2589 Compare October 30, 2024 10:55
@xrmx xrmx marked this pull request as ready for review October 30, 2024 10:56
@xrmx xrmx requested review from a team as code owners October 30, 2024 10:56
@xrmx
Copy link
Contributor Author

xrmx commented Oct 30, 2024

@drewby @lmolkova I have rebased this, PTAL!

@xrmx xrmx force-pushed the add-genai-system-fingerprint branch from 48d2589 to 04a2ce3 Compare October 30, 2024 10:58
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Thank you for following up on this one!
LGTM (but will need to regenerate tables once upstream heals)

@nirga
Copy link
Contributor

nirga commented Oct 31, 2024

@xrmx can you rebase? I think it will fix the CI failure

@xrmx xrmx force-pushed the add-genai-system-fingerprint branch from bba715f to 58c9429 Compare October 31, 2024 09:01
@xrmx
Copy link
Contributor Author

xrmx commented Oct 31, 2024

Rebased and regenerated table

@lmolkova lmolkova enabled auto-merge (squash) November 1, 2024 02:59
@lmolkova lmolkova merged commit 520702f into open-telemetry:main Nov 1, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants