-
Notifications
You must be signed in to change notification settings - Fork 913
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
Add gen_ai metrics to AWS Bedrock instrumentation #13408
Conversation
} | ||
|
||
static void applyClientOperationDurationAdvice(DoubleHistogramBuilder builder) { | ||
if (!(builder instanceof ExtendedDoubleHistogramBuilder)) { |
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.
Followed the pattern of the other metrics classes but this post-dates me. IIUC, api-incubator has to be on the classpath to prevent cardinality explosion - is that OK?
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.
opentelemetry-instrumentation-api
has an implementation dependency on opentelemetry-api-incubator
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.
Got it - I guess it means it would only not work for an alternative SDK which probably we don't expect in practice.
Should we log an error before return
?
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.
Should we log an error before
return
?
makes sense 👍
I think I'm going to need to figure out a way bribe two other languages to implement setAttributesAdvice
so that I can get it marked stable in the spec. As you noted, we are super reliant on this behavior in Java instrumentation.
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.
I realized perhaps the others don't do it for the same thing I ran into, where we wouldn't want to log if it's no-op, but there doesn't seem to be a great way to check for that on a metric builder. Let me leave it as-is following others for now and think some more on it.
…nstrumentation into bedrock-genai-metrics
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.
Well done. Much smaller since base infra is in. This shows with the same precanned response, we get metrics like we do in openai.
Would it be possible to get a review on this? Thanks! |
if (state == null) { | ||
logger.log( | ||
FINE, | ||
"No state present when ending context {0}. Cannot record database operation metrics.", |
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.
replace database operation
with whatever is appropriate
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.
Oops, thanks fixed
Follows up to #13355 with metrics. Mostly handled by the instrumentation API
/cc @codefromthecrypt