-
Notifications
You must be signed in to change notification settings - Fork 647
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
Add TCP/HTTP Metrics #790
Add TCP/HTTP Metrics #790
Conversation
Let's refine separately to see if we can inline some handlers with existing handlers |
|
||
static { | ||
boolean micrometerCheck; | ||
try { |
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.
Could Use Reactor Core Metrics.isAvailable method
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.
done
@@ -26,6 +26,7 @@ | |||
import java.util.function.Supplier; | |||
import javax.annotation.Nullable; | |||
|
|||
import io.micrometer.core.instrument.MeterRegistry; |
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.
careful as its optional dep
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.
done
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.
some tiny tweaks
Related to #157
Codecov Report
@@ Coverage Diff @@
## master #790 +/- ##
============================================
+ Coverage 66.55% 67.09% +0.54%
- Complexity 1383 1429 +46
============================================
Files 133 137 +4
Lines 6371 6705 +334
Branches 830 871 +41
============================================
+ Hits 4240 4499 +259
- Misses 1685 1741 +56
- Partials 446 465 +19
Continue to review full report at Codecov.
|
.description("Number of the error that are occurred") | ||
.tags(REMOTE_ADDRESS, address, URI, address + request.uri()) | ||
.register(registry) | ||
.record(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.
If this always records a value of 1
, a Counter
is probably appropriate. Am I missing any reason to use a DistributionSummary
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.
Switch to Counter e7f9378
Thanks
METHOD, request.method().name(), | ||
STATUS, response.status().codeAsText().toString()) | ||
.register(registry)); | ||
dataSentTimeSample.stop(responseTimeBuilder.tags(URI, address + request.uri(), |
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.
The dataSentTimeSample
was already stopped in the write
method. What's the reason for recording it again 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.
The timer is different. I will rename the Timer.Sample so that it is obvious.
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.
address = socketAddress.toString(); | ||
} | ||
|
||
dataSentTimeSample.stop(dataSentTimeBuilder.tags(URI, address + request.uri(), |
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.
Due to cardinality and usability, I would leave the address out of the URI
tag. For reference, here is the documentation on the tags Spring Boot uses for HTTP clients: https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-metrics.html#production-ready-metrics-http-clients
Even the URI itself can be unbounded in cardinality when path variables are used (e.g. /books/{bookId}
with many different values of bookId
). Is there any way in Reactor Netty to specify a template URI with path variables? Regardless, allowing high cardinality for tag values is not good as it will use increasingly more memory in the application and the metrics backend will also suffer. Micrometer offers a MeterFilter
to limit the maximumAllowableTags
, as can be seen used here in Spring Boot: https://github.com/spring-projects/spring-boot/blob/543fcdbbfdc807dd4cc70934b076756d80e2d21c/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/client/HttpClientMetricsAutoConfiguration.java#L53-L61
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.
Due to cardinality and usability, I would leave the address out of the URI tag.
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.
Micrometer offers a MeterFilter to limit the maximumAllowableTags
This one I'll extract in a separate issue #794
Thanks
DistributionSummary.builder(name + DATA_RECEIVED) | ||
.baseUnit("bytes") | ||
.description("Amount of the data that is received, in bytes") | ||
.tags(REMOTE_ADDRESS, remoteAddress, URI, PROTOCOL) |
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.
tcp
is already in the (default) name of the metric, so I wonder if tagging uri=tcp
adds anything. It also may confuse users who think to use it like the HTTP metrics' uri
tag.
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.
The name
depends on whether you run TCP(HTTP) server or client.
If you run HTTP server for example, without uri=tcp
, there will be metrics: 'reactor_netty_http_server_data_sent_bytes' with address only and with address and uri.
Prometheus will complain with the following:
java.lang.IllegalArgumentException: Prometheus requires that all meters with the same name have the same set of tag keys. There is already an existing meter named 'reactor_netty_http_server_data_sent_bytes' containing tag keys [remote_address]. The meter you are attempting to register has keys [remote_address, uri].
So for HTTP server and client you have metrics per uri and on tcp level. They might differ because per uri only the incoming/outgoing body is measured (without headers) while on TCP level - everything.
Related to #157