Skip to content
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

Reactor-Netty Metrics #19388

Closed
michaelmcfadyen opened this issue Dec 17, 2019 · 16 comments
Closed

Reactor-Netty Metrics #19388

michaelmcfadyen opened this issue Dec 17, 2019 · 16 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@michaelmcfadyen
Copy link
Contributor

Since the release of reactor-netty 0.9.0.RELEASE, both the server and the client will now collect several groups of metrics (TCP/UPD/Conenction Pool etc). These are enabled by invoking a .metrics() on the server/client.
This is one of the related issue on reactor-netty (reactor/reactor-netty#797)

Should we update the configuration of the netty web server to enable these metrics by default? Is there a way to enable these metrics by default as well on all web clients?

The default metrics library used by reactor-netty is micrometer.

HttpServer Docs: https://projectreactor.io/docs/netty/release/api/index.html?reactor/netty/http/server/HttpServer.html
TcpClient Docs: https://projectreactor.io/docs/netty/release/api/index.html?reactor/netty/tcp/TcpClient.html
HttpClient Docs: https://projectreactor.io/docs/netty/release/api/reactor/netty/http/client/HttpClient.html

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 17, 2019
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 17, 2019
@bclozel bclozel added this to the 2.3.x milestone Dec 17, 2019
@bclozel
Copy link
Member

bclozel commented Dec 17, 2019

Note: we should wait for Reactor Netty 0.9.3.RELEASE, because of reactor/reactor-netty#931 and reactor/reactor-netty#926.

@violetagg
Copy link
Member

@bclozel Currently we have maximumAllowableTags for uri by default 100
Do you need a way for customising it?
https://github.com/reactor/reactor-netty/blob/master/src/main/java/reactor/netty/http/MicrometerHttpMetricsRecorder.java#L53

@michaelmcfadyen
Copy link
Contributor Author

michaelmcfadyen commented Jan 11, 2020

For reference, the existing client and server metrics created by spring boot have a max uri tag value that also defaults to 100. The developer can then override this value via properties.

If we're following the same pattern here, then there would need to be a way of customising that value.

@violetagg
Copy link
Member

@michaelmcfadyen reactor/reactor-netty#950

@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Jan 24, 2020
@michaelmcfadyen
Copy link
Contributor Author

I believe this is now unblocked by the release of reactor-netty 0.9.5.RELEASE

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Mar 12, 2020
@michaelmcfadyen
Copy link
Contributor Author

I've been trying to think of some acceptance criteria for this issue and have came up with the following.

HttpServer:

  • metrics enabled by default (maybe using a NettyServerCustomizer)
  • can be disabled via an config prop
  • NettyServerCustomizer will be auto configured or not based on the config prop

HttpClient:

  • unsure if this can be defaulted in spring boot as WebClient.Builder and ReactorClientHttpConnector are in spring framework. These classes control the default configuration of the netty HttpClient.

Thoughts?

@bclozel bclozel self-assigned this Mar 23, 2020
@michaelmcfadyen
Copy link
Contributor Author

Noticed this issue raised around a memory issue metrics in reactor netty. This may be another blocker before integrating into spring boot.
reactor/reactor-netty#1010

@bclozel bclozel added the status: blocked An issue that's blocked on an external project change label Mar 31, 2020
@bclozel
Copy link
Member

bclozel commented Mar 31, 2020

Indeed, the current arrangement is storing data in unbounded collections and using tags (raw URIs) that are likely to create cardinality explosion.

@philwebb philwebb removed the status: blocked An issue that's blocked on an external project change label Apr 17, 2020
@bclozel
Copy link
Member

bclozel commented Apr 26, 2020

See reactor/reactor-netty#1075

@bclozel bclozel modified the milestones: 2.3.x, 2.3.0.RC1 Apr 26, 2020
@bclozel
Copy link
Member

bclozel commented Apr 26, 2020

I've just closed this issue, here are a few notes about the current state of metrics support.

First, we've only enabled TCP and buffer allocation metrics in Reactor Netty (so no HTTP metrics) for several reasons:

  1. similar metrics are already recorded at the WebFlux level (client and server) so we should not duplicate data here
  2. HTTP metrics are all tagged with URI tag information which causes cardinality explosion if we don't provide a function that transforms expanded URIs into templated ones. At the WebFlux level, this infrastructure doesn't make any sense - this is only useful for direct Reactor Netty users
  3. the two points above show that producing HTTP metrics would quickly reach limits (a metrics filter is configured to avoid memory issues) and is of limited use in the Spring Boot context

Also, metrics support in Reactor Netty is quite new and there might still be performance issues/bugs (see reactor/reactor-netty#1075). Right now, Reactor Netty does not allow to publish metrics to a custom registry, but rather to the global registry only.

@violetagg
Copy link
Member

@bclozel Please check whether the fix reactor/reactor-netty#1076 for reactor/reactor-netty#1075 is relevant for the integration that is committed for this issue.
We are still discussing but most probably REMOTE_ADDRESS will be removed for the HTTP server metrics and also the metrics on protocol level will be disabled.
I think that you enabled metrics on protocol level for the server.

@bclozel
Copy link
Member

bclozel commented Apr 27, 2020

Thanks for the heads up @violetagg

#1076 doesn't seem to affect our support here; having less tags is easier than removing them once they've been published, so we're good here.

@violetagg
Copy link
Member

violetagg commented Apr 27, 2020

956afdc#diff-fc6a2fac46dd5d9f6d194074364855baR42
here you enable TCP level (i.e. protocol) metrics, with a lot of HTTP clients this will lead to a cardinality explosion as they have REMOTE_ADDRESS tag.
Wdyt?

@bclozel
Copy link
Member

bclozel commented Apr 27, 2020

I thought it was fixed by #1076 already?
Why cardinality explosion doesn't apply for raw TCP communications as well?

@bclozel
Copy link
Member

bclozel commented Apr 27, 2020

Reopening as it doesn't seem we can use the TCP metrics either.

@bclozel bclozel reopened this Apr 27, 2020
@bclozel bclozel removed this from the 2.3.0.RC1 milestone Apr 27, 2020
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement labels Apr 27, 2020
@bclozel
Copy link
Member

bclozel commented Apr 27, 2020

I'm declining this enhancement feature since it seems that it's not really useful for Spring Boot applications.

On top of my previous comment, it seems that TCP metrics are only meant for raw TCP clients/servers and that there is no allocator metrics specific API (which is provided by Netty). I'm declining this enhancement as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

6 participants