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(bedrock): support metrics for bedrock #1957

Merged
merged 12 commits into from
Oct 2, 2024

Conversation

jinsongo
Copy link
Contributor

@jinsongo jinsongo commented Sep 12, 2024

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 12, 2024
@jinsongo jinsongo marked this pull request as draft September 12, 2024 04:52
@dosubot dosubot bot added new instrumentation python Pull requests that update Python code labels Sep 12, 2024
@jinsongo jinsongo changed the title Support metrics for bedrock feat(bedrock): support metrics for bedrock Sep 12, 2024
@jinsongo jinsongo force-pushed the bedrock-metrics branch 2 times, most recently from 83ca96c to df14a45 Compare September 17, 2024 20:15
Copy link

gitguardian bot commented Sep 17, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Revert "Update"

This reverts commit 0214db8.

Update

Update metrics tests and cassettes file
@jinsongo jinsongo marked this pull request as ready for review September 18, 2024 05:48
@dosubot dosubot bot added the testing label Sep 18, 2024
@jinsongo
Copy link
Contributor Author

jinsongo commented Sep 18, 2024

I already know the root cause why I always keep getting the error botocore.errorfactory.AccessDeniedException: An error occurred (AccessDeniedException) when calling the InvokeModel operation: You don't have access to the model with the specified model ID.. Thanks.

@jinsongo jinsongo force-pushed the bedrock-metrics branch 2 times, most recently from 040b0c8 to 6f5a11b Compare September 19, 2024 08:49
@@ -29,6 +29,8 @@ class Meters:
LLM_WATSONX_COMPLETIONS_RESPONSES = "llm.watsonx.completions.responses"
LLM_WATSONX_COMPLETIONS_TOKENS = "llm.watsonx.completions.tokens"

LLM_BEDROCK_COMPLETIONS_EXCEPTIONS = "llm.bedrock.completions.exceptions"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required?

Copy link
Contributor Author

@jinsongo jinsongo Sep 20, 2024

Choose a reason for hiding this comment

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

It's an open issue. I think it's better to use a common name, for example llm.completions.exceptions for all ai system to count exceptions. If yes, I would like to use another PR to fix all related code.

Copy link
Member

Choose a reason for hiding this comment

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

@jinsongo if we're doing this here I'd try to use the new genAI conventions - https://opentelemetry.io/docs/specs/semconv/gen-ai/gen-ai-metrics
I think they're already released so you can even use them directly from the original otel semconv package

Copy link
Member

Choose a reason for hiding this comment

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

(in this case - it means exceptions are counted in the duration metric)

Copy link
Contributor Author

@jinsongo jinsongo Sep 21, 2024

Choose a reason for hiding this comment

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

@nirga I cannot find an appropriate metric name for the exceptions counter of Bedrock. As you know, version 0.4.1 of opentelemetry-semantic-conventions-ai does not include LLM_BEDROCK_COMPLETIONS_EXCEPTIONS although it's already included in this current PR commits, which causes the build to fail when I use it in the Bedrock instrumentation code. Actually, that's the same approach for OpenAI, Authropic and Watsonx.

Copy link
Member

@nirga nirga Sep 21, 2024

Choose a reason for hiding this comment

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

@jinsongo this should be in the standard opentelemetry-semantic-conventions package. And you should count exceptions on the duration metric as specified in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jinsongo If this parameter is not required, I think we can ignore this for now, and we can fix this in future as a consolidation for semantic convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gyliu513 I have removed LLM_BEDROCK_COMPLETIONS_EXCEPTIONS from semconv_ai, and added a TODO comment about fixing in future as a consolidation for semantic convention.

@jinsongo jinsongo force-pushed the bedrock-metrics branch 2 times, most recently from 19ed21a to 9e1110e Compare September 23, 2024 23:59
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

nice @jinsongo! a couple of styling comments and we're ready to go

@gyliu513
Copy link
Contributor

/lgtm

@nirga can you help review? Thanks

@jinsongo
Copy link
Contributor Author

@nirga Do you have more comments? Thanks.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 2, 2024
@nirga nirga merged commit a30bb8c into traceloop:main Oct 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer python Pull requests that update Python code size:L This PR changes 100-499 lines, ignoring generated files. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants