-
Notifications
You must be signed in to change notification settings - Fork 1
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
Response time OTel metric #223
Conversation
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.
LGTM, nice refactor/renaming to boot, thanks @raphael-theriault-swi! Not sure if you've ad-hoc tested to confirm that the histograms are indeed DELTA aggregation, I think Python and Java had to explicitly set that.
transaction: string | undefined, | ||
) { | ||
const method = span.attributes[SemanticAttributes.HTTP_METHOD] | ||
const statusCode = span.attributes[SemanticAttributes.HTTP_STATUS_CODE] |
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.
Something I didn't even think about until now ;p -- does the JS lambda instrumentation always represent a lambda invocation as an HTTP server span, i.e. we always get the method and status_code? Back when adding support in AO it was a horror to find out the way a function is invoked produces different request context payload and only the "via API Gateway" (which now the Function URL makes easy) had HTTP type info in the payload.
Not a blocker for this PR, just curious.
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'll need to double check
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.
Looks like these attributes are actually never set for Lambda spans at the moment
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.
OK, as discussed at standup the metric would just be tagged with whatever attributes are available then.
Adds the response time metric