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

Documentation for custom observation conventions is missing important hints #40158

Closed
hadjiski opened this issue Apr 3, 2024 · 9 comments
Closed
Labels
for: external-project For an external project and not something we can fix status: invalid An issue that we don't feel is valid

Comments

@hadjiski
Copy link

hadjiski commented Apr 3, 2024

Considering a standard spring boot rest controller application with some business and data repository layers + outgoing client communication:

Customizing observation conventions might get tricky, when you go beyond the trivial well documented examples in the spring boot docu, spring framework docu and micrometer

Here my thoughts:

  • customization is not consistent: even for the commonly used metrics like http.server.requests, http.client.requests, spring.data.repository.invocations, tasks.scheduled.execution you either extend some default convention or define a bean of a tags customizer etc.
  • most of the time folks want to customize the tags dynamically based on some business data (e.g. business entity, customer type, etc.). This data is often not part of the servlet request as parameter, body or header, rather collected as part of the business logic during the execution of the particular rest controller call. Now when it comes to propagating the data to the observation convention customizers, it tricky, cause:
    • in case of http.server.requests the data can be added to the servlet request's attributes in the business layer, which are then available in the customizer in the ServerRequestObservationContext -> getCarrier()
    • in case of http.client.requests the data can be added to the observation registry's current observation context's map in the business layer, which are then available in the customizer in the ClientRequestObservationContext map
    • in case of spring.data.repository.invocations the data can be added to the observation registry's current observation context's map in the business layer, which are then available in the customizer in the same way (observation registry's current observation context's map), because the carrier is not of any such use
    • in case of tasks.scheduled.execution the data can be added to the observation registry's current observation context's map in the business layer, which are then available in the customizer in the ScheduledTaskObservationContext map
      (as you can see, for the "top" 4 metrics, we have 3 different ways of propagating business data)
      I know people promote using of context propagation lib, which I did not study so far deeply, but it seems like an overhead for the small thing we are trying to achieve here. Not to mention the fact, that without the extra lib, we can rely on the context being reset for us, not worrying about clearing all the thread locals properly

It would be nice to align those, but I assume it is in the nature of things, when many projects are building on top of each other - then let it be better documented with examples

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 3, 2024
@philwebb
Copy link
Member

philwebb commented Apr 3, 2024

@jonatan-ivanov Do you have any thoughts on this one? I'm not sure that anyone on the Boot team has the experience to suggest how these things could be done. I'm also not sure where any extended documentation should live if it's needed.

@philwebb philwebb added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Apr 3, 2024
@bclozel
Copy link
Member

bclozel commented Apr 4, 2024

Solutions 2) 3) and 4) all look similar to me:

  • getting the current observation
  • adding information to its Observation.Context directly and using those in the ObservationConvention
  • OR adding KeyValues directly to the Observation.Context as low/high cardinality

The fact that you can use Servlet request attributes as a context map is irrelevant here and I think this information really belongs to the Micrometer documentation, if it's not already there. I'm closing this issue as a result.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2024
@bclozel bclozel added status: invalid An issue that we don't feel is valid for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Apr 4, 2024
@hadjiski
Copy link
Author

hadjiski commented Apr 4, 2024

@bclozel, let me give you some concrete "running" code, so you can see the similarities, but also the differences. It took me some time to find a minimal approach, which seems to serve lots of cases and I think it is worth documenting it in some way:

...
@Autowired
private final ObservationRegistry observationRegistry;
...

Propagating...

public void pushBusinessDataToObservation(<some business data>) {

        // this makes sure that the current context (if available) will persist the business data
        Optional.ofNullable(observationRegistry.getCurrentObservation())
                .ifPresent(observation -> 
                                        observation.getContext().put("someKey", <some business data>));

        // this makes sure that the current http servlet request (if available) 
        // will persist the business data
        Optional.ofNullable(RequestContextHolder.getRequestAttributes())
                .ifPresent(requestAttributes -> 
                                       requestAttributes.setAttribute("someKey", <some business data>));
    }

Consuming...

public KeyValues getLowCardinalityKeyValuesFromCarrierAttributes(ServerRequestObservationContext context) {

        // this serves only http.server.requests metric and it is the only way to do it,
        // since during the pushing of the business data the current observation is a child 
        // of the http.server.requests one,
        // so propagating it via the http servlet request appears to be the only out of the box option

        return KeyValues.of(
                Optional.ofNullable(context)
                        .map(c -> c.getCarrier()
                                .getAttribute("someKey")
                        .orElse(<empty>)
                        .toArray(new String[0])
        );
    }

    public KeyValues getLowCardinalityKeyValuesFromContextMap(Observation.Context context) {

        // this serves all the cases, where a proper context object is accessible within the customizer
        // this is the case for: http.client.requests and tasks.scheduled.execution
        // we better use this provided context, since for example for tasks.scheduled.execution
       // the observationRegistry.getCurrentObservation() is 'null', so we lookup through the context chain
       // where is our business data

        MetricEnrichment found = null;

        ContextView currentObservationContextView = context;
        while (currentObservationContextView != null) {

            if ((found = currentObservationContextView.get("someKey")) != null) {

                break;
            }

            currentObservationContextView = 
                Optional.ofNullable(currentObservationContextView.getParentObservation())
                    .map(ObservationView::getContextView)
                    .orElse(null);
        }

        return KeyValues.of(
                Optional.ofNullable(found)
                        .orElse(<empty>)
                        .toArray(new String[0])
        );
    }

    public Iterable<Tag> getLowCardinalityKeyValuesFromCurrentContextMap() {

        // this serves all the cases, where no context is accessible within the customizer
        // this is the case for: spring.data.repository.invocations and all the mongodb.driver ones
        // this time, the current observation is properly put to the scope,
       // so the observationRegistry.getCurrentObservation() is available and 
       // we can lookup through its context chain where is our business data

        MetricEnrichment found = null;

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

            if ((found = currentObservationContextView.get("someKey")) != null) {

                break;
            }

            currentObservationView = currentObservationContextView.getParentObservation();
        }

        return Tags.of(
                Optional.ofNullable(found)
                        .orElse(<empty>)
                        .toArray(new String[0])
        );
    }

@jonatan-ivanov
Copy link
Member

most of the time folks want to customize the tags dynamically based on some business data (e.g. business entity, customer type, etc.). This data is often not part of the servlet request as parameter, body or header, rather collected as part of the business logic during the execution of the particular rest controller call.

I think if some data is missing from the Context, please let the project that instrument itself know so that they can add data to the Context that you can use. I feel though here it's not the case, if this data is collected as part of the business logic you might want to create a separate observation for that business logic and put whatever you want into the context.

as you can see, for the "top" 4 metrics, we have 3 different ways of propagating business data

I might not understand this but I think they are the same: add data to the Context somewhere and use that data elsewhere (I think Brian sees this the same way).

Looking at your code also tells me that you might want a new Observations for your business logic. Also, can't you do this instead:

observation.lowCardinalityKeyValues("someKey", someValue);

@hadjiski
Copy link
Author

hadjiski commented Apr 5, 2024

@bclozel @jonatan-ivanov thanks for your comments

I think if some data is missing from the Context, please let the project that instrument itself know so that they can add data to the Context that you can use. I feel though here it's not the case

No no, you might have gotten me wrong, I don't miss data in the context, I just look for consistent elegant way to access dynamic business data during the customization of the tags, which naturally means, that it has to be pushed to the observation context manually beforehand like you mentioned here:

if this data is collected as part of the business logic you might want to create a separate observation for that business logic and put whatever you want into the context.

Though I did not want to solve it via extra observation (thanks for the suggestion), because this would add the "ugly" overhead of wrapping all code places with observation.observe(() -> anyBsinessMethod(context));

I might not understand this but I think they are the same: add data to the Context somewhere and use that data elsewhere (I think Brian sees this the same way)

Yes, that is true, but where to add it resp. how to consume it differs (as already mentioned):

  • http.server.requests's context is a parent's parent one of the business layer one (spring.security.http.secured.requests), so adding the business data to the context is not working as we cannot add data to the parent contexts and the only way is to pass it to the http servlet request, which is available in the business layer and also available in the customizer later as it is appended as a carrier
  • for http.client.requests and tasks.scheduled.execution we can perfectly utilize directly the current context as they live within the same context as the business layer and the context is available in the customizers
  • the spring data metrics are not even initiating a new observation, rather directly a meterRegistry metric, so no context is available in the customizers, but we still can utilize the current context just getting it via the autowired observationRegistry
    (btw. when it comes to pure metrics and their low cardinality values, so no traces, is using the observation.observe(...) in rough simple words just an equivalent to manually defining all the meters like counter, gauge or there is a difference between initiating a monitoring via an observation vs via a meterRegistry)

And exactly this I would like to see documented with code examples, which would save time researching through the whole monitoring topic.

Also, can't you do this instead

No, this appears not possible as the business logic is mostly within the spring.security.http.secured.requests context (in a standard rest controller spring boot app), so adding custom tags directly to the current observation in the business layer will end up in the wrong metric

@bclozel
Copy link
Member

bclozel commented Apr 5, 2024

@hadjiski I think that when it comes to collecting information somewhere in the application and attaching it to an observation, you have 3 choices:

  1. add it as a new KeyValue to the observation, if the required information is already in the observation context. If it's not, as @jonatan-ivanov suggests, it might be something that is missing. If the information is really about "business code", then probably not.
  2. add the business information to the Observation.Context and then use it in the observation convention to add KeyValues.
  3. create a custom observation

If the information is local to the current request and is a cross cutting concern, like a customer id - it makes sense to add it to some ThreadLocal and apply it globally to many observations using an observation filter. If the information is even more global and completely static for the lifetime of the application, there are easier ways to globally contribute that information to all observations.

If this information is local to some business logic, then a dedicated observation probably makes sense. You shouldn't have to wrap all calls like observation.observe(() -> anyBsinessMethod(context)), but instead instrument business methods internally. You would then get the correlation with the current HTTP request and other data through tracing.

In all cases, I think that adding business data to server request attributes and using it as a carrier, out of band, to collect information for other observations, is not a great idea. It indeed looks like a missing custom observation in the application. At least this is how it looks with the information you've provided.

(btw. when it comes to pure metrics and their low cardinality values, so no traces, is using the observation.observe(...) in rough simple words just an equivalent to manually defining all the meters like counter, gauge or there is a difference between initiating a monitoring via an observation vs via a meterRegistry)

Yes, but Observation handlers will produce timers and traces. Counters and gauges don't really make sense with the Observation API. You could try to implement a custom Observation Handler to do this but I think you would easily see the limitations to that approach. For those, the Metrics API is still the way to go.

I don't think we can improve the documentation as the issue is not really about learning how to do something, but choosing a solution to collect the relevant information and thinking about the tradeoffs.

@divyagangigunta123
Copy link

@hadjiski .. I have integrated my spring boot application with cassandra observability. Have configured it and able to push low cardinality metrices to prometheus. But unable to push high cardinality keys to prometheus with the existing code base.

Can you please let me know which class need to be overridden and how to allow db.statement into low cardinality ? This will be helpful if you can please suggest on this

@bclozel
Copy link
Member

bclozel commented Apr 26, 2024

@divyagangigunta123 this is by design. High cardinality key values should not be published with metrics as metrics systems are not meant to collect unbounded values. Please check the Micrometer docs for further information.

@jonatan-ivanov
Copy link
Member

@divyagangigunta123 On top of what Brian mentioned, you can read about this more and why this is a problem here: https://develotters.com/posts/high-cardinality/
Also, this is done by DefaultMeterObservationHandler, you can override its behavior if you want to but I would highly recommend against it in production since if you have high cardinality tags, either your application or your metrics backend or both will suffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

6 participants