-
Notifications
You must be signed in to change notification settings - Fork 646
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
Capture HTTP/2 server active stream metrics #2357
Conversation
a9d1f64
to
59d0d23
Compare
@samueldlightfoot We need to exclude these two new default methods from the compatibility check. Please add to
|
59d0d23
to
d57a8eb
Compare
Build failure on (windows-2019, nio). Not sure if this is a transient error or a real issue. @violetagg |
This is a flaky test and not relevant to this change. |
@reactor/netty-team PTAL |
public String getName() { | ||
return "reactor.netty.http.server.active.streams"; | ||
} |
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.
for the existing http server metrics, we currently have for example:
- reactor.netty.http.server.connections.active
- reactor.netty.http.server.connections.total
etc ...
We use the same prefix, followed by the meaning of the meter (active, total, etc ...)
so, for consistency reasons, wouldn't it be better to use :
reactor.netty.http.server.steams.active instead of reactor.netty.http.server.active.streams ?
thanks.
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 agree - I'd aligned with the client-side counterpart 'reactor.netty.connection.provider.active.streams', but think it makes sense to be consistent within the server metrics.
42467eb
to
0ed0372
Compare
0ed0372
to
e6209ef
Compare
@pderop PTAL |
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
thanks @samueldlightfoot !
This is already in 1.1.0-M4 with PR #2357
…trib main branch.
@samueldlightfoot Thanks for the PR I back ported the feature in |
Capture H2 active streams metrics for HttpServer.
Issue: #2356