Allow observation to have a mutable access to parent

[hadjiski]

Normal Spring boot 3 application with a rest controller, business layer and a spring data repository involving mongodb driver.
This ends up having the following observations auto-configured (among others):
http.server.requests
http.client.requests
spring.data.repository.invocations
mongodb.driver.commands
tasks.scheduled.execution

For all besides the "http.server.requests" the current observation is the "spring.security.http.secured.requests" with parent "spring.security.filterchains" with root parent "http.server.requests"

The "http.server.requests" has no parent.

Now when I customize tags for these metrics, typically some beans are overridden or interfaces implemented and inside we either have access to the observation context (e.g. for the http.server/client.requests) or not (e.g. for the spring.data.repository.invocations), so one can not write nicely a reusable code how to propagate business data from the business layer into the context and read it out inside the customizers to decide on the custom tags.
So my idea was to consistently put my business data into the ObservationRegistry.getCurrentObservation().getContext().put(...) and then inside the customizer to read it out, but here is the problem:
Inside the customizer for http.server.requests the current observation context is a parent of the one, which was available at the time the business data was propagated into the context.
Well, this can be solved by propagating the business data to the HttpServletRequest attributes, which are then available in the observation's context carrier, but this already makes it a different approach compared for example to the spring data repository or mongodb command customizers, where you only have access to some custom event object and not the observation context.
So I was thinking, why not propagating the business data always to the root parent, so that no matter which context is available in the customizers, I just need to lookup in their root parents and will find it.
Though this is considered a bad practice and blocked by 3423.

Here is a concrete example, how it can be solved by the current spring and micrometer capabilities:

• propagate business data to the current observation context and current http servlet request attributes
• in any of the customizers, lookup for availability of
  • current observation context and all of its parents
  • http servlet request
  • provided server request context

@RestController
public class SomeRestController {
...
@GetMapping(...)
@ResponseStatus(OK)
public String getPaymentMethods() {
    ...
    observationService.enrichMetricWith(...);
    ...
    // further down is the service layer, data repository, outgoing clients etc.
}

@Component
public class ServerRequestMetricsCustomizer extends DefaultServerRequestObservationConvention {
...
    @Override
    public KeyValues getLowCardinalityKeyValues(ServerRequestObservationContext context) {
        return super.getLowCardinalityKeyValues(context)
                 // context has to be passed as the only way to access the propagated business data
                .and(observationService.getMetricEnrichmentAsKeyValues(context));
    }
}

@Component
public class ClientRequestMetricsCustomizer extends DefaultClientRequestObservationConvention {
...
    @Override
    public KeyValues getLowCardinalityKeyValues(ClientRequestObservationContext context) {
        return super.getLowCardinalityKeyValues(context)
                 // context does not need to be passed as accessing the business data works fine 
                 // via the current observation context
                .and(observationService.getMetricEnrichmentAsKeyValues(null));
    }
}

@Component
public class SpringDataRepositoryMetricsCustomizer extends DefaultRepositoryTagsProvider {
...
    @Override
    public Iterable<Tag> repositoryTags(RepositoryMethodInvocation invocation) {
        return ((Tags) super.repositoryTags(invocation))
                              .and(observationService.getMetricEnrichmentAsTags());
    }
}

@Component
public class MongoDBCommandMetricsCustomizer extends DefaultMongoCommandTagsProvider {
...
    @Override
    public Iterable<Tag> commandTags(CommandEvent event) {
        return ((Tags) super.commandTags(event)).and(observationService.getMetricEnrichmentAsTags());
    }
}

public class ScheduledTaskMetricsCustomizer extends DefaultScheduledTaskObservationConvention
        implements GlobalObservationConvention<ScheduledTaskObservationContext> {
...
    @Override
    public KeyValues getLowCardinalityKeyValues(ScheduledTaskObservationContext context) {
        return super.getLowCardinalityKeyValues(context)
                .and(observationService.getMetricEnrichmentAsKeyValues());
    }
}


@Service
public class ObservationService {

@Autowired
private ObservationRegistry observationRegistry;
...
public void enrichMetricWith(<business data>) {
        // propagate to the current observation and the http servlet request

        Optional.ofNullable(observationRegistry.getCurrentObservation())
                .ifPresent(observation -> observation.getContext().put("enrichmentKey", <business data>));

        Optional.ofNullable(RequestContextHolder.getRequestAttributes())
                .ifPresent(requestAttributes -> requestAttributes.setAttribute(
                            "enrichmentKey", <business data>, ServletRequestAttributes.SCOPE_REQUEST));
    }

public Iterable<Tag> getMetricEnrichmentAsTags() {
...
// same as getMetricEnrichmentAsKeyValues, just returning tags
}

public KeyValues getMetricEnrichmentAsKeyValues(
                               ServerRequestObservationContext serverRequestObservationContext) {
        // lookup first in all observation contexts from current to top
        // then fallback to lookup in the http servlet request
        // then fallback to lookup in the provided from outside context

        foundMetricEnrichment = null;

        ObservationView currentObservationView = observationRegistry.getCurrentObservation();
        while (currentObservationView != null) {
            ContextView currentObservationContextView = currentObservationView.getContextView();

            if ((foundMetricEnrichment = currentObservationContextView.get("enrichmentKey")) != null) {

                break;
            }

            currentObservationView = currentObservationContextView.getParentObservation();
        }

        foundMetricEnrichment = Optional.ofNullable(foundMetricEnrichment)
                .orElse(Optional.ofNullable(RequestContextHolder.getRequestAttributes())
                        .map(requestAttributes ->
                                requestAttributes
                                        .getAttribute("enrichmentKey", 
                                                            ServletRequestAttributes.SCOPE_REQUEST))
                        .orElse(foundMetricEnrichment));

        return Optional.ofNullable(foundMetricEnrichment)
                .orElse(Optional.ofNullable(serverRequestObservationContext)
                        .map(context -> context.getCarrier()
                                .getAttribute("enrichmentKey"))
                        .orElse(<none key values to not jeopardize prometheus metric labels>))
                .getEnrichmentValues();
    }
}

Though, if I would have had access to the parent observations to be able to put into their contexts, then I could strip down the code to only put into the top parent context and lookup there as well, without these fallbacks.

Btw. why is it so different to customize the tags for the different metrics:

• extending DefaultServerRequestObservationConvention // works like a charm
• extending DefaultClientRequestObservationConvention // works like a charm
• extending DefaultRepositoryTagsProvider // works like a charm though differs from first two ones
• extending DefaultMongoCommandTagsProvider // works like a charm though differs from first two ones
• implementing GlobalObservationConvention<ScheduledTaskObservationContext> // work only after manually adding it as part of a custom bean configuration of the ObservationRegistryCustomizer<ObservationRegistry>...observationRegistry.observationConfig().observationConvention(...)

I understand that the question is tightly coupled to the spring framework as well, but it is influenced by the micrometer designs I guess

[ttddyy]

If the parent context is mutable, one can change the parent context name, tags, etc.
However, the end result for such changes is very much dependent on the ObservationHandler implementation.

For example, in DefaultMeterObservationHandler, a long task timer is created at onStart specifying the tags.
If a tag is added to the observation context later via the parent relationship, the added tags will not be reflected in this long task timer.
On the other hand, there is a timer started/stopped at onStart/onStop. This timer is registered to the meter registry at the stop. Therefore, changes to the context name or tags will be reflected.

So, when the parent context is modified, what to happen depends on the implementation of ObservationHandler - how it uses the Observation.Context.
I don't think it is a good idea to expose such implementation dependent behavior by making the parent context mutable.

Though, there are certain use cases that is benefitial to modify the parent context based on the business logic.
For example, if a business logic decides the received http request is a fraud request, it may be a nice feature to tag the request observation/metrics/span denoting it is identified as a fraud.

In such a case, I think implementing ObservationConvention is one way to achieve it without modifying the parent context.
This way, as you have mentioned, you need to implement multiple ObservationConventions for each type of observation context to support.

Another way could be implementing a custom ObservationHandler where you can react to multiple types of observations.

public class MyObservationHandler implements MeterObservationHandler<Observation.Context> {

    // modify "supportsContext" method to restrict context types to support

    @Override
    public void onStart(Context context) {
	Timer.Sample sample = Timer.start(this.meterRegistry);
	context.put(Timer.Sample.class, sample);
        ...
    }

    @Override
    public void onStop(Context context) {
        // retrieve the result determined by the business layer
	boolean isFraud = FraudResultHolder.get();  // for example, get from a thread local variable
        ...
        Timer.Sample sample = context.getRequired(Timer.Sample.class);
	sample.stop(Timer.builder(context.getName()).tags(tags).register(this.meterRegistry));
        ...
    }

}

[hadjiski]

I understand your point completely.
Though wouldn't it be beneficial to expose just the putting to the context map, so one could easily propagate business data, which can be later used to decide on custom tags for example

[ttddyy]

While re-reading the issue description and code snippet, I misunderstood the intention.

So I was thinking, why not propagating the business data always to the root parent, so that no matter which context is available in the customizers, I just need to lookup in their root parents and will find it.

Initially, I thought you wanted to modify the parent context to change the outcome of the parent context (metrics or spans for the parent). But, I realized you are just using the parent context to keep the data accessible to child contexts.

Instead of going that way, I think it is more appropriate for each child context/convention to directly access the necessary data, for example via thread local variable.
If it hops multiple threads, then I recommend looking into the context-propagation library to make the thread local variable propagated in the different threads.

[hadjiski]

Thanks for the suggestion, though it appears a bigger overhead to me than the current inconsistent approach I constructed, cause one has to deal with properly resetting those thread locals, which is out of the box given for us, when relying on the observation contexts.