Add parameter to customize status codes mapped as 'NOT_FOUND' in OkHttpMetricsEventListener

[tangcent]

Problem Description

In my project, I frequently request resources via URLs formatted as /resource/{id}. Some resources may not exist or may have expired, resulting in a 404 response. It's crucial for me to monitor the proportion of such invalid (possibly expired) resources. Using the urlMapper functionality of OkHttpMetricsEventListener, I convert these URLs from /resource/xxxxx to a templated form /resource/{id} for consistent metric collection. But the current implementation aggregates all 404 responses under a generic NOT_FOUND tag, which obfuscates the visibility of specific URLs or patterns contributing to these errors.

As discussed in #2410, the OkHttpMetricsEventListener in Micrometer currently handles specific HTTP status codes, such as 404, by mapping them to a NOT_FOUND tag to mitigate tag cardinality issues. However, this behavior is fixed and does not allow for customization based on user-specific needs or different application contexts.

Proposed Solution

Introduce flexibility in the OkHttpMetricsEventListener that enables users to define custom sets of status codes that should be treated as NOT_FOUND. Additionally, provide the option to configure whether to create URI tags for requests resulting in these status codes, which could include or exclude 404 among others.

Benefits

• Customization: Users gain control over how status codes are handled, allowing them to adapt metrics collection to better suit their operational and monitoring needs.

• Reduced Complexity: Provides a way to handle exceptions to general tagging rules without adding significant complexity to the monitoring setup.

• Enhanced Usability: Makes the OkHttpMetricsEventListener more versatile and suitable for a wider range of use cases by accommodating different monitoring strategies.

This change would make the OkHttpMetricsEventListener more adaptable to various monitoring needs, enhancing its overall utility and effectiveness in real-world scenarios.

[shakuzen]

Did you see my comment on the linked issue?

After looking into this a bit, I think it is already possible to configure, since we added the contextSpecificTags in 1.5. See this tag method on the Builder where you can configure a function to overwrite the uri tag.

Would that not be sufficient for your use case and customizing this behavior?

[shakuzen]

@tangcent I'm still curious about my previous question, when you have a chance to respond.

[tangcent]

@shakuzen sorry I missed your previous message. yes, using contextSpecificTags to overwrite the uri tag is indeed a workable solution. but I also agree with @johntowell's point:

I don't understand why unbounded cardinality tag explosion concern is specific to the 404 error code? This concern seems to be the responsibility of the uriMapper implementer. It's easy to create unbounded cardinality tag explosion with any poor implementation of uriMapper that does not filter out ids, why would this be any different for error code 404? 404 is a perfectly acceptable restful way to return not found for a given resource.

After reviewing the code again, I think a better approach would be to add a switch that allows users to decide whether to process the uri for APIs with response codes 404 and 301 as NOT_FOUND. this would provide more flexibility and clarity compared to configuring statusCodesMappedAsNotFound.

While contextSpecificTags seems like a silver bullet, it makes me consider whether it's valuable to refactor the uriMapper as follows:

        private BiFunction<Request, Response, String> uriMapper

        public Builder uriMapper(Function<Request, String> uriMapper) {
            this.uriMapper = (request, response) -> uriMapper.apply(request);
            return this;
        }

        public Builder uriMapper(BiFunction<Request, Response, String> uriMapper) {
            this.uriMapper = uriMapper;
            return this;
        }

what do you think?

[shakuzen]

We started discussing this internally. It probably makes the most sense to get rid of the special handling for 404/301 on the client side instrumentation. It makes sense to have such special handling on the server side instrumentation, but the same reasoning doesn't apply on the client side. I don't remember exactly why it was there on the client side instrumentation to begin with. We should strive to be consistent among our different HTTP client instrumentations, though, so I think it may be best to make an overall tracking issue for this and sub issues for the change in each instrumentation. We also have the Observation-based instrumentation of each client now as well.

We'll keep you posted on what we decide to do, but I presume if the logic for the 404/301 status responses goes away or is moved to the default URL mapper, that would simplify implementing your use case, right?

[tangcent]

@shakuzen yes, that makes sense! thanks for the clarification. I’ll go ahead and close this PR. lmk if there's anything else I can do. :)

[shakuzen]

I've opened #5812 to track the larger change.