-
Notifications
You must be signed in to change notification settings - Fork 524
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
Expose HTTP/gRPC metrics to OpenTelemetry as well #11564
Conversation
This pull request does not have a backport label. Could you fix it @dmathieu? 🙏
NOTE: |
} | ||
|
||
func (m *monitoringMiddleware) getMetric(n request.ResultID) metric.Int64Counter { | ||
name := "http.server." + n |
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 named those metrics http.server.xxx
, and not apm-server.http.server.xxx
, as the former better matches the otel semantic conventions.
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md
We can rename those metrics in the local exporter.
expectedOtel: map[string]int64{ | ||
"grpc.server." + string(request.IDRequestCount): 1, | ||
"grpc.server." + string(request.IDResponseCount): 1, | ||
"grpc.server." + string(request.IDResponseErrorsCount): 1, |
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.
question: why do we have an additional IDResponseErrorsInternal
in monitoringInt ?
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 haven't really figured out why, but nothing can set it from this interceptor, so I suspect it's because something elsewhere sets it, only for monitoring.Int.
The only location that sets it though is jaeger, and it's not executed here. So it may be state from a previous test?
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 have removed the metric from monitoring.Int, and the test doesn't fail either. So it does seem it's not being incremented in that interceptor (and the monitoring.Int test doesn't check for metrics that aren't emitted).
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.
Didn't dive too deep into the technical details, but overall approach LGTM
* add otel metrics to HTTP middleware * add otel metrics to gRPC interceptor * add server to metrics names * remove unnecessary assignment * remove metrics that doesn't appear to be used
Tested via #11582 (comment) |
Motivation/summary
This modifies the HTTP and gRPC metrics middlewares to expose data to OpenTelemetry as well, so we can expose those metrics to the local exporter.
Checklist
apmpackage
have been made)For functional changes, consider:
How to test these changes
This is unit-tested. It can also be tested manually by running apm-server with Tilt and self instrumentation enabled.
Related issues