-
Notifications
You must be signed in to change notification settings - Fork 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
Record Jetty bytes in/out metrics through network listeners #4514
Conversation
8aac231
to
9d5a65b
Compare
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed after this change we're measuring the bytes through NetworkTrafficListener that hooks in whenever actual in / out bytes are being processed fixes gh-3873
9d5a65b
to
753988f
Compare
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'm not sure about the difference in the number of bytes counted. It would be good to understand that. The description of the metrics should be updated as well.
connector.addBean(new JettyConnectionMetrics(registry)); | ||
JettyConnectionMetrics metrics = new JettyConnectionMetrics(registry); | ||
connector.addBean(metrics); | ||
connector.addNetworkTrafficListener(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.
This looks like new setup that isn't documented in the class JavaDoc or done in the static addToAllConnectors
method. It will be a behavior change that the bytes in/out will only be available if this is set, right?
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.
Yes, that's correct plus I'm also showing this in the tests that bytes are not equal to what they were used to. So not only do you need to set it up differently but also the result is different
@joakime do you have some time to help us understand this? When changing from counting the bytes in/out on the connection to a network listener and we're getting different numbers. |
Depending on where the integration point is in the handler tree you will either see the bytes as initiated by the webapp, or the bytes as sent on the network. The most obvious and largest difference would be if a GzipHandler is in place. The techniques that micrometer were using in Jetty 9/10/11 were only seeing the bytes passing through the HttpOutput / HttpInput classes. Using the
While in the techniques you used in Jetty 9/10/11 on micrometer you saw only the Body before Transfer-Encoding. |
Thanks for the explanation. One effect of changing from counting the bytes via |
I agree, it would be nice to have HttpClient also participate in NetworkTrafficListener ... So I opened jetty/jetty.project#11491 to do just that! |
Thank you for that. I've subscribed to the issue. We can update our tests/documentation when that is available. |
before this change we were dumping in / out bytes from the connection upon the connection close which could result in big traffic whenever the connection got closed after this change we're measuring the bytes through
NetworkTrafficListener
that hooks in whenever actual in / out bytes are being processedfixes gh-3873