Ordering of ObservationHandlers

[foaw]

I would like to override the default remote service name, for which there isn't a property. As far as I can tell, the least code invasive way of doing this is with handlers. However, in the current code, the remote service name is set when an observation is started, in PropagatingSenderTracingObservationHandler#createSenderSpan. So if I want to override it, I have to do it before the tracer picks it up.

The main ways out of this situation I see are:

• Don't rely on the bundled customiser and register an ObservationRegistry bean of my own.
• Implement not ObservationHandler but TracingObservationHandler so that it falls into the category of tracers. The downside is that I will now have to delegate calls to the actual TracingObservationHandler that I'm taking the place of.
• Subclass PropagatingSenderTracingObservationHandler and delegate all calls to the necessary handlers first, then do tracing. The default bean has @ConditionalOnMissingBean, so there shouldn't be problems registering it.

Obviously, I want to stick with the default configurer and change as little code as possible. However, I cannot declare my own categories, ObservationHandlers don't respect Ordered#getOrder or an equivalent thereof, and the categorised handlers are registered first.

I suggest (1) exporting ObservationHandlerGrouping as part of the public API and (2) introducing a proper mechanism for ordering handlers. WDYT?

[wilkinsona]

Can you please describe your use case in some more detail? I'm not sure how a globally configured default service name would be useful as it needs to be service-specific. Much of the observation support already sets such a service name. For example, Spring Data MongoDB's MongoObservationCommandListener sets it to mongo and Spring AMQP's RabbitMessageSenderContext sets it to RabbitMQ.

[foaw]

What brought me here was Kafka, where the remote service name includes the cluster ID. While I do agree it's a good idea to include it somewhere, I don't think the service name is exactly the best place; I'd rather put into low-cardinality tags. However, I wouldn't focus on renaming too much, as ordering may come in handy in many other case.

As for the global service name, my handler filters out types of context that it's not interested in, specifically that aren't instances of KafkaRecordSenderContext, so it isn't really a global one.

[jonatan-ivanov]

Modifying these in a handler might not be a good idea (even if you can do it, the intention of the concept of a handler is different).

As far as I understand, you should do this from an ObservationFilter since its job is providing you a way to modify Observations but unfortunately in this case you cannot do this in an ObservationFilter right now since the remoteServiceName is set before ObservationFilter is called.
(The remoteServiceName of the Span is set from onStart while ObservationFilter is called right before onStop.)

I think there might be solutions other than using an ObservationHandler but some might need changes in Micrometer to make them work:

1. Maybe the simplest solution: we have a SpanFilter which is for modifying every span before reporting, it can modify the remoteServiceName and Boot auto-configures it so you "just" need to create a @Bean.
2. We could modify the behavior of Micrometer and set the remoteServiceName in onStop instead of onStart in the propagating tracing handlers. That would mean the ObservationFilter can modify the context before the remote name is set which I think would be a nice feature anyway and it would solve your issue.
3. The propagating tracing handlers have customizeSenderSpan/customizeExtractedSpan/customizeReceiverSpan methods. You should be able to override them (they are noop by default) and create your custom tracing handlers. I think Boot should auto-configure those in the right order as a drop-in replacement of the current handlers.
4. We could also make customizers injectable and call them in these methods. Boot could inject these customizers if they present which means you need to create a customizer bean.

I think I would not focus on the ObservationHandlers and their order to solve this since customizing the Observation Context should not be their responsibility, you should be able to do it in an ObservationFilter (in this case it does not work but I think we can fix it) or in a SpanFilter (in this case it should work).

[foaw]

Many libraries don't conform to the (sometimes experimental) OpenTelemetry conventions, and RestClient/RestTemplate and Feign are basically inconsistent. This isn't surprising since they were developed independently of each other, but normally we expect parts of one application doing the same job to behave similarly.

This is to say that it's not just Kafka where I wish it behaved differently—it just so happens that Kafka is the only one where I can do it myself without involving any third parties. Once I'm done implementing tracing as possible right now, I'll to move on, and there will barely be any changes to my code later on.

Ideally, all libraries should "just" follow the conventions. Right now, to catch up with the recommendations, this will require opening lots of issues, pointing out all the inconsistencies, and most likely writing the code and "accompanying" it until it's merged. Given the number of repositories, I alone won't have that much time. 😔

---

As for your suggestions:

• I have somehow overlooked CompositeSpanExporter—many thanks for pointing it out! I think I'll continue with this option for now.

• This only deals with remoteServiceName, which in my opinion is too narrow. Nice to have, but makes little difference in the big picture.

• I have already tried this before, but decided it was an overkill for the task. It's my (3) too in the issue, by the way. 😃

• Great idea if we're talking metrics where there isn't "MetricFilter". Also, to stay consistent with tracing naming, maybe not customizers, but Reporters, Filters, etc. This is where I'd like to see the code before deciding for myself, but yeah.

Now I see there is a tool more suitable for what I need, but why not give a way in general to order handlers, if I understand your position right?

[jonatan-ivanov]

I think many don't use ("just" follow) the OTel semantic conventions because OTel semantic conventions are not stable yet. RestClient/RestTemplate and Feign aren't using it either because their had conventions before OTel and they are backward compatible with those (and because OTel semantic conventions are not stable yet).

Btw the Observation API lets you define the convention using ObservationConvention; we are planning to support the OTel semantic conventions as they stabilize (fyi: HTTP is stable, we need to investigate).

This only deals with remoteServiceName, which in my opinion is too narrow. Nice to have, but makes little difference in the big picture.

You should be able to modify every detail by using an ObservationFilter or an ObservationConvention. Not being able to override remoteServiceName is a bug that we should fix.

Great idea if we're talking metrics where there isn't "MetricFilter". Also, to stay consistent with tracing naming, maybe not customizers, but Reporters, Filters, etc. This is where I'd like to see the code before deciding for myself, but yeah.

I'm not sure I get this, #4 is basically the same as #3 but without the need of inheritance. :)

Now I see there is a tool more suitable for what I need, but why not give a way in general to order handlers, if I understand your position right?

This is on the Boot team to decide, but I'm not sure we should introduce extra complexity if it is not necessary anywhere.

[wilkinsona]

I'm not sure we should introduce extra complexity if it is not necessary anywhere

This is the Boot team's opinion too. Until there's a compelling use case for ordering handlers that cannot be achieved by some other means, it will be very hard to justify adding support for it. The cost both in terms of initial development and ongoing maintenance would be better spent elsewhere.

We can reconsider in the future but I'm going to close this one for now. Thanks anyway for the suggestion.