Use user-provided ObservationRegistries instead of constant global registry

[tmarback]

Currently, reactor-netty's instrumentation with Micrometer's Observation API relies on a constant global ObservationRegistry created within netty itself (in Metrics.java). It should instead allow client code to inject their own ObservationRegistry instances to the components being used.

Motivation

Using a fixed registry makes reactor-netty's instrumentation not useable for tracing if the user already has an external registry under use (for example, the one provided by Spring Boot Actuator), as attempting to use both registries at once leads to incorrect results (from my experience it leads to inconsistent issues with parent spans, missing spans, etc). According to the Micrometer developers, this is not a bug, as only one registry should be in use in the app lifecycle.

Desired solution

Ideally, each major component (e.g. a Client or Server instance, other independent internal components) should be able to receive an ObservationRegistry instance from client code to use in their metrics, for maximum configurability. Incidentally, this would bring it more in line with Reactor itself, which has moved away from global registries altogether recently, even for metrics.

Considered alternatives

A limited but simpler alternative patch would be to allow client code to set the global registry used by netty (possibly by moving it behind a static getter/setter pair). This retains many of the limitations so it's not the preferred solution, but it could be a potential temporary workaround if the changes required for per-instance registries would take a significant time (so the instrumentation is at least usable by apps with only one registry in the meantime). I can provide a PR with this option if desired.

[violetagg]

@tmarback Can you give us more details? Why do you need to enable both Spring Boot observations and Reactor Netty observations?

[tmarback]

My application uses Reactor Netty's HTTPClient to make requests to some external APIs it uses. I would like to have visibility into those requests within an operation (traced through observations) as they are often a big chunk of the runtime, so logically it would be nice to use the instrumentation that already exists. That said, there are other components in the application (like the database) that also need to be traced, and those are either set up by Spring Boot directly, or by the application using the Spring-provided registry (as a regular IoC dependency).

[violetagg]

@tmarback What do you think about specifying ObservationRegistry as a Bean?

@Bean
ObservationRegistry observationRegistry(Tracer tracer, Propagator propagator) {
	OBSERVATION_REGISTRY.observationConfig()
			.observationHandler(
					new ObservationHandler.FirstMatchingCompositeObservationHandler(
							new ReactorNettyPropagatingSenderTracingObservationHandler(tracer, propagator),
							new ReactorNettyTracingObservationHandler(tracer),
							new DefaultTracingObservationHandler(tracer)));
	return OBSERVATION_REGISTRY;
}

This is the configuration that you need, Spring Boot will add the rest to this ObservationRegistry.

[tmarback]

@violetagg I'm not quite sure what it is, but something about doing it that way still breaks the final trace:

For reference, this is how it looks normally:

If I go the other way around and compile a local version of reactor-netty where the global registry is settable and use that to inject the ObservationRegistry from Spring, it works: (note the http.post spans):

Worth noting that the last image doesn't add ReactorNettyPropagatingSenderTracingObservationHandler or ReactorNettyTracingObservationHandler (whatever handlers Actuator adds by default seem to be sufficient), so it's possible it has something to do with handlers, but I'm not sure.

[violetagg]

In order to have Reactor Netty tracing working you have to have both ReactorNettyPropagatingSenderTracingObservationHandler and ReactorNettyTracingObservationHandler
More in the documentation: https://projectreactor.io/docs/netty/release/reference/index.html#_tracing_4

[violetagg]

can you provide some reproducible example?

[tmarback]

But that's what I'm saying, it did work without those handlers (after replacing the global netty registry with the Spring Boot-provided one). Adding a few GETs still shows up, for example:

And they seem to have all the tags I would expect them to have.

I'll try to set up an example repo but it might take a bit.

[violetagg]

But that's what I'm saying, it did work without those handlers (after replacing the global netty registry with the Spring Boot-provided one). Adding a few GETs still shows up, for example:

And they seem to have all the tags I would expect them to have.

In your previous snapshots there were no connect, tls handshake and hostname resolution. Can you confirm that without those configurations these measurements are there?

I'll try to set up an example repo but it might take a bit.

That would be great! Thanks!

[tmarback]

In your previous snapshots there were no connect, tls handshake and hostname resolution. Can you confirm that without those configurations these measurements are there?

Ah, sorry about that, I forgot to mention that the http.posts are using a long-running TCP connection so they don't have those stages 🙃

But yes, all those measurements are there even without ReactorNettyPropagatingSenderTracingObservationHandler/ReactorNettyTracingObservationHandler (only whatever handlers Spring Boot Actuator sets up)

[tmarback]

Also, sidenote that this is all somewhat beside the point; even if overriding the default ObservationRegistry bean with OBSERVATION_REGISTRY worked with no issues, it doesn't feel like a particularly great long term solution given that it still restricts how the application can configure the tracing system, which arguably partially defeats the point of using Micrometer. It also leaves some other problematic scenarios; for example, if another library (say, a database driver) decided to also use a fixed global registry of their own for their instrumentation it would make it functionally incompatible with reactor-netty for tracing purposes, which wouldn't be great.

[violetagg]

Also, sidenote that this is all somewhat beside the point; even if overriding the default ObservationRegistry bean with OBSERVATION_REGISTRY worked with no issues, it doesn't feel like a particularly great long term solution given that it still restricts how the application can configure the tracing system, which arguably partially defeats the point of using Micrometer. It also leaves some other problematic scenarios; for example, if another library (say, a database driver) decided to also use a fixed global registry of their own for their instrumentation it would make it functionally incompatible with reactor-netty for tracing purposes, which wouldn't be great.

Agree, I would like to see what we can offer you as a workaround for the time being.

Worth noting here that if you want to use all optimisations and functionality provided by Reactor Netty, you need to configure your new Observation Registry by yourself.

reactor-netty/reactor-netty-core/src/main/java/reactor/netty/Metrics.java

Lines 43 to 48 in 0d8277c

	static {
	OBSERVATION_REGISTRY.observationConfig().observationHandler(
	new ObservationHandler.FirstMatchingCompositeObservationHandler(
	new ReactorNettyTimerObservationHandler(REGISTRY),
	new DefaultMeterObservationHandler(REGISTRY)));
	}

ReactorNettyTimerObservationHandler contains optimisations for Timers creation.
ReactorNettyPropagatingSenderTracingObservationHandler/ReactorNettyTracingObservationHandler specifies the exact tags that will be provided to the spans. Can you check the span tags, do you see duplications?

[tmarback]

ReactorNettyPropagatingSenderTracingObservationHandler/ReactorNettyTracingObservationHandler specifies the exact tags that will be provided to the spans. Can you check the span tags, do you see duplications?

Looking at the source, they seem to be only copying the tags present in the context rather than directly specifying them. That said, I compared the span tags with OBSERVATION_REGISTRY bean vs replacing OBSERVATION_REGISTRY with SB's registry and as far as I can tell the tags are the same. No duplication on either case, but I'm not sure if tags in the same span can be duplicated anyway.

By the way, I made the sample reproducible case: https://github.com/tmarback/netty-observation-repro

This is what I see after running it (as per the README):

[violetagg]

@tmarback There is an issue in Spring Boot when you want to add custom ObservationHandlers to the configuration
spring-projects/spring-boot#34510

[violetagg]

@tmarback We are going to address this issue for the next release.

You wrote:

According to the Micrometer developers, this is not a bug, as only one registry should be in use in the app lifecycle.

and

Ideally, each major component (e.g. a Client or Server instance, other independent internal components) should be able to receive an ObservationRegistry instance from client code to use in their metrics

Do you have a use case where you need more than one ObservationRegistry when in one JVM?

[tmarback]

@violetagg Sorry for the delay.

We are going to address this issue for the next release.

Awesome, will be waiting for it 👍

Do you have a use case where you need more than one ObservationRegistry when in one JVM?

Personally I don't have one; I suggested per-component registry injection as that is what I understood to be the recommended approach by the Micrometer devs when I asked on Slack (though it's always possible I misunderstood), and I didn't see any reason why it shouldn't be supported (other than a relatively small code reduction).

CC @jonatan-ivanov

[violetagg]

I'm not challenging the need of API for changing the ObservationRegistry. I'm challenging whether this has to be done per server/client/connection pool.

1. If we have an API on the level of reactor.netty.Metrics then all component in one JVM, whether it is the server, the client or the connection pool will work with one and the same ObservationRegistry. (similar to when you declare a Bean in Spring Boot, your server and client do not need to specify the ObservationRegistry with a configuration on the level of server or client).

2. If we need a configuration on the level of the server, client, connection pool then:

• You need to configure explicitly every one of these
• This API cannot be exposed on the level of reactor-netty-core and reactor-netty-http because these modules have optional dependency on Micrometer and have to able to work without Micrometer in the path. This means that we need two additional modules reactor-netty-micrometer-core and reactor-netty-micrometer-http where we have to move our integration with Micrometer. This is challenging to be done in a patch/minor release.

[tmarback]

I'm not challenging the need of API for changing the ObservationRegistry. I'm challenging whether this has to be done per server/client/connection pool.

I know, that's what I was talking about. What I meant is that the impression I got is that the "recommended" way to do things is, in addition to having a settable registry in general, to have per-component granularity, which would imply that there is some known use case for it (even if I don't personally know about it). But again, I'm not sure.

This API cannot be exposed on the level of reactor-netty-core and reactor-netty-http because these modules have optional dependency on Micrometer and have to able to work without Micrometer in the path.

Fair enough, I didn't consider that detail. I guess arguments could be made about the practical benefits of making a facade dependency optional, but I'm sure this has already been debated by people much more knowledgeable than me :)

This is challenging to be done in a patch/minor release.

I alluded to this in the original issue, but I'm perfectly OK with sticking with a global-but-settable registry in the immediate term as that already fixes the main problem. Everything else can be just a long-term consideration.

[jonatan-ivanov]

Personally I don't have one; I suggested per-component registry injection as that is what I understood to be the recommended approach by the Micrometer devs when I asked on Slack (though it's always possible I misunderstood), and I didn't see any reason why it shouldn't be supported (other than a relatively small code reduction).

It would help if you could point us to this discussion but injecting a registry per-component does not mean injecting a new registry to each component: quite the opposite, it should be one registry that you should inject everywhere you need. I see multiple reasons why this should not be supported, the strongest one to me is the inability of tracking the current observation so context propagation (and tracing) won't work (it would also make somewhat of a chaos throughout your components).

[tmarback]

It was this Slack thread. You mentioned

let their components use an external registry that the user of the lib can inject

Which, given the context (of there not being an official global registry), I interpreted to mean per-component injection. If I got that wrong, then that's my bad 🙃

does not mean injecting a new registry to each component: quite the opposite, it should be one registry that you should inject everywhere you need.

Of course, I don't mean that every component would have a different registry (the whole reason I opened this issue was that reactor-netty was using a different registry than Boot, after all). The question was whether there is a use case to allowing major independent components to have different registries (say, two distinct HTTP clients that are used in entirely distinct flows but happen to be in the same JVM), versus maintaining the global (i.e. static) registry that netty currently uses and just make its value replaceable.

[jonatan-ivanov]

Sorry for not being clear, by "an external registry" I meant one registry.
There is actually one interesting use-case where you can have more registries but this should be done by components that are instrumenting themselves and not really user code:

If you have a component where you can't inject a registry (static utility class/method) and if you need the current observation, since the thread local for it is static, if you are in the same class loader and create an instance of the registry in your static component, you will be able to get the current observation. But I would not consider this as the normal use of the Observation API since this should not really happen in user code and that registry should only be used for getting the current observation and for creating any.

[tmarback]

That solves that issue then, thank you for clarifying 🙂

[violetagg]

@tmarback @jonatan-ivanov Thanks for the clarification!