-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
query/receive: Unify HTTP server instrumentation #1458
Conversation
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
9233a14
to
850c716
Compare
c2551ab
to
e34285f
Compare
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
e34285f
to
7222659
Compare
ins := extpromhttp.NewNopInstrumentationMiddleware() | ||
if o.Registry != nil { | ||
ins = extpromhttp.NewInstrumentationMiddleware(o.Registry) | ||
o.Registry.MustRegister(h.forwardRequestsTotal) |
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.
Did we always forget to register this or am I missing why this line is here?
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.
If I understood you correctly, It was registered before. I just removed the ones which come from InstrumentationMiddleware
.
Lines 112 to 119 in 8feb079
if o.Registry != nil { | |
o.Registry.MustRegister( | |
h.requestDuration, | |
h.requestsTotal, | |
h.responseSize, | |
h.forwardRequestsTotal, | |
) | |
} |
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. Makes sense now.
Sorry misclicked. Didn’t mean to approve, just comment. |
This PR attempts to unify HTTP server instrumentation behavior across components and removes unnecessary metrics. Thanos Query and Receive now use common instrumentation middleware.
PTAL #1420 for previous discussions.
Changes
thanos_query_api_instant_query_duration_seconds
,thanos_query_api_range_query_duration_second
metrics.thanos_http_request_duration_seconds
,thanos_http_requests_total
,thanos_http_response_size_bytes
.Verification
Local
make test
and run.