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

Add request to Observation Context at creation to enable filtering by the request #12854

Open
jonatan-ivanov opened this issue Mar 8, 2023 · 12 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement

Comments

@jonatan-ivanov
Copy link
Member

This issue was originally discussed in spring-projects/spring-boot#34400.
The scenario is the following: if Spring Security is used and users want to ignore /actuator endpoints, there is no easy way to ignore Observations created by Spring Security because the details to make this decision are missing when the ObservationPredicate is tested.

One potential solution would be adding the request to the Context before the Observation is created, one example here:

private AroundFilterObservation parent(HttpServletRequest request) {
FilterChainObservationContext beforeContext = FilterChainObservationContext.before();
FilterChainObservationContext afterContext = FilterChainObservationContext.after();
Observation before = Observation.createNotStarted(this.convention, () -> beforeContext, this.registry);
Observation after = Observation.createNotStarted(this.convention, () -> afterContext, this.registry);
AroundFilterObservation parent = AroundFilterObservation.create(before, after);
request.setAttribute(ATTRIBUTE, parent);
return parent;

Would be something like:

FilterChainObservationContext beforeContext = FilterChainObservationContext.before(request);
FilterChainObservationContext afterContext = FilterChainObservationContext.after(request);

Expected behavior: having extra details on the Observations created by Spring Security that allows the users to ignore these Observation based on the request.

There is a reproducer in the issue mentioned above.

@jzheaux
Copy link
Contributor

jzheaux commented Mar 28, 2023

Thanks for the suggestion, @jonatan-ivanov. Can you elaborate on how the request details would be read when evaluating the ObservationPredicate?

Based on spring-projects/spring-boot#34400 I wonder if it should add http.url.

@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Mar 28, 2023
@jonatan-ivanov
Copy link
Member Author

It can be an uri field on the context, a tag or for full flexibility, does the whole request make sense?

I think something like this:

@Bean
ObservationPredicate actuatorServerContextPredicate() {
    return (name, context) -> {
        if (name.startsWith("spring.security.") && context instanceof SecurityObservationContext secObsContext) {
                return !secObsContext.getCarrier().getRequestURI().startsWith("/actuator");
        }
        return true;
    };
}

But as an uri field this would be quite similar: secObsContext.getRequestURI().startsWith("/actuator")

Here is a working example excluding the actuator http spans.

@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2023

In that case, would someone that is trying to turn off spans by request need to know how specifically how to turn off each project's spans? I'm wondering, for example, if FilterChainObservationContext should instead implement some sort of request-aware observability interface.

Perhaps I'm not seeing the whole picture, but from where I'm standing, I'm seeing an application that needs to know Spring Security specifics -- as well as the specifics of other modules -- to perform an observability-level task of filtering out spans from a request.

@braunsonm
Copy link
Contributor

In that case, would someone that is trying to turn off spans by request need to know how specifically how to turn off each project's spans?

That's the exact problem I have with this solution. And once that this change in micrometer will also not solve: micrometer-metrics/micrometer#3678

It seems like the only way to do this properly is as mentioned on the original issue, by adding an attribute on every span you have in order to ignore them with a predicate.

(From Jonathan on the original issue)

// This adds the http.url keyvalue to security observations from the root (mvc) observation
// You add an ignoreSpan=true keyValue instead if you want, or something that can signal to the SpanExportingPredicate what to ignore
@Bean
ObservationFilter urlObservationFilter() {
    return context -> {
        if (context.getName().startsWith("spring.security.")) {
            Context root = getRoot(context);
            if (root.getName().equals("http.server.requests")) {
                context.addHighCardinalityKeyValue(root.getHighCardinalityKeyValue("http.url"));
            }
        }

        return context;
    };
}

private Observation.Context getRoot(Observation.Context context) {
    if (context.getParentObservation() == null) {
        return context;
    }
    else {
        return getRoot((Context) context.getParentObservation().getContextView());
    }
}

// This ignores actuator spans but its logic depends on the ObservationFilter above
// Auto-configuration for SpanExportingPredicate was added in 3.1.0-M1
// So either you use 3.1.x or you can add the same to your config : https://github.com/spring-projects/spring-boot/pull/34002
@Bean
SpanExportingPredicate actuatorSpanExportingPredicate() {
    return span -> !span.getTags().get("http.url").startsWith("/actuator");
}

@jonatan-ivanov
Copy link
Member Author

In that case, would someone that is trying to turn off spans by request need to know how specifically how to turn off each project's spans?

Filtering out spans from anywhere in the flow can be a valid use-case I think (this is about Observations but that does not change much), I think saying that http Observations are off for actuator but Observation XYZ that is triggered by that HTTP call is not is a completely valid use-case though I feel not as frequent.

@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2023

Filtering out spans from anywhere in the flow can be a valid use-case

Yes, that's the case I'm referring to. I imagine something like the following:

  1. Look up the request in the context or its ancestry
  2. Make a decision based on the request material

Or, in pseudocode, something like this:

ObservationPredicate isNotActuator = (name, context) -> {
    RequestAwareObservationContext requestAware = context.getFirstAncestorOfType(RequestAwareObservationContext.class);
    return requestAware == null || !requestAware.getCarrier().getRequestURI().endsWith("/actuator");
};

I'm not sure what other complexities I am ignoring or unaware of, so please take the code as just an observation.

Regardless of the best way to solve this, I think it would be better to expose the request in a non-Spring-Security-specific way.

@jonatan-ivanov
Copy link
Member Author

jonatan-ivanov commented Mar 30, 2023

@braunsonm Could you please try out one thing?
I talked to Josh who came up with the idea of getting the current request through RequestContextHolder.getRequestAttributes(). Since the "parent" http observation is attached to it, you can use it to filter out actuator, or any observations that was triggered by an HTTP request but its parent http observation is a noop (disabled):

@Bean
ObservationPredicate noRootlessHttpObservations() {
    return (name, context) -> {
        RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
        if (requestAttributes instanceof ServletRequestAttributes servletRequestAttributes) {
            Observation observation = (Observation) requestAttributes.getAttribute(ServerHttpObservationFilter.class.getName() + ".observation", SCOPE_REQUEST);
            return observation == null || !observation.isNoop();
        }
        return true;
    };
}

@braunsonm
Copy link
Contributor

braunsonm commented Mar 31, 2023

@jonatan-ivanov Appears to work! Here is a crude example of using it for the original use case of ignoring actuator calls

    @Bean
    ObservationPredicate noRootlessHttpObservations() {
        return (name, context) -> {
            RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
            if (requestAttributes instanceof ServletRequestAttributes servletRequestAttributes) {
                Observation observation = (Observation) requestAttributes.getAttribute(ServerHttpObservationFilter.class.getName() + ".observation", SCOPE_REQUEST);
                return  (!servletRequestAttributes.getRequest().getRequestURI().startsWith("/actuator")) || (observation == null || !observation.isNoop());
            }

            // The root observation is not stored in the `RequestContextHolder` in this stage and needs to be handled separately
            if (context instanceof ServerRequestObservationContext serverContext) {
                return (!serverContext.getCarrier().getRequestURI().startsWith("/actuator"));
            }
            
            return true;
        };
    }

Unfortunately I don't think this solution is portable to Webflux though. I think you'd have to use ServerWebExchangeContextFilter in place of the RequestContextHolder. But in order to get the context you would need the ObservationPredicate to be reactive.

@betalb
Copy link

betalb commented Oct 3, 2023

It seems that provided code doesn't work if management context is a child context (when it is running on separate port).

There are 2 issues:

  • root span http.server.requests (i.e. http get /actuator/health), is not created at all, but spring-security creates 3 root spans (before, request, after)
  • RequestContextFilter is not registered, so RequestContextHolder doesn't have anything

From what I was able to find, when management server is started as child context, it has slightly separate configuration, that doesn't register RequestContextFilter if it sees other bean of that type in context. The problem is that condition sees bean of this type in context, but this bean is not registered for management context.

Child Context configuration:

https://github.com/spring-projects/spring-boot/blob/f947bad3f73ff0e330b6986b4b3ef0d8a726659f/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/web/servlet/WebMvcEndpointChildContextConfiguration.java#L106

As a workaround I've added own configuration that adds required filter and registers this configuration in org.springframework.boot.actuate.autoconfigure.web.ManagementContextConfiguration.imports. This allows me to apply above code, but if someone wants full trace for actuator endpoint, root span issue should be resolved differently.

@jonatan-ivanov
Copy link
Member Author

root span http.server.requests (i.e. http get /actuator/health), is not created at all, but spring-security creates 3 root spans (before, request, after)

Are you doing any kind of filtering to disable this?
If not and you think it should be there, could you please open an issue for Spring Framework with a minimal Java reproducer that demonstrates the issue?

@betalb
Copy link

betalb commented Oct 3, 2023

No, not doing any filtering. As self-check I've simply restarted app with management port set equal to main port and span was created as expected.

Should I create ticket for spring-framework or spring-boot? It looks like specific to actuator.

@jonatan-ivanov
Copy link
Member Author

Framework has the instrumentation, I would start there but now that you mentioned that you are setting the management port, I think this might be a known issue but I don't find it.
@bclozel Could you please help us out? with the above? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
Status: In Progress
Development

No branches or pull requests

4 participants