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

Provide configuration for ignoring routes from being sampled #34400

Closed
braunsonm opened this issue Feb 27, 2023 · 19 comments
Closed

Provide configuration for ignoring routes from being sampled #34400

braunsonm opened this issue Feb 27, 2023 · 19 comments
Labels
for: external-project For an external project and not something we can fix theme: observability Issues related to observability

Comments

@braunsonm
Copy link

braunsonm commented Feb 27, 2023

Right now, you have 3 options for configuring if a trace is reported.

  1. Using the probability configuration properties, which only give you control over the percentage reported

  2. Using a ObservationRegistryPredicate, however there doesn't seem to be a way to ignore any spans which are created as a result of a HTTP request because subsequent spans do not have an http.url attribute you can match on.

  3. Providing a custom Sampler. However the shouldSample method does not receive the final attributes for the span and thus you cannot determine what path the sample is for.

Ideally Spring would provide a way to configure this. The primary use case being to ignore any traces related to /actuator since these typically are only for health checks and clutter up the tracing backend.

If there is another method I have not thought of that would work, I'd love to hear it!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 27, 2023
@philwebb philwebb added the theme: observability Issues related to observability label Feb 28, 2023
@mhalbritter
Copy link
Contributor

mhalbritter commented Mar 3, 2023

There's a way, but it's not pretty:

@Configuration(proxyBeanMethods = false)
class MyObservationConfig {
	@Bean
	ObservationPredicate predicate() {
		return (s, context) -> {
			if (context instanceof ServerRequestObservationContext serverRequestObservationContext) {
				HttpServletRequest carrier = serverRequestObservationContext.getCarrier();
				return !carrier.getServletPath().startsWith("/actuator");
			}
			return true;
		};
	}
}

The Context would be fully populated inside an ObservationFilter, but i can't find a way to prevent the observation being reported from ObservationFilter.

@jonatan-ivanov Do you know any way?

@mhalbritter mhalbritter added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Mar 3, 2023
@braunsonm
Copy link
Author

braunsonm commented Mar 3, 2023

@mhalbritter unfortunately as I mentioned in the original issue that doesn't work. At least it hasn't in my experience.

The first span will be able to get ignored that way but any subsequent observations will still be reported. Inside the predicate there doesn't seem to be a way to get the parent span so you can't recursively find out if a new observation is related to a request.

For example with that predicate, we can still see a call with a WebClient or RestTemplate get reported to our trace backend, since they are not using the HttpServletRequestContext

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Mar 3, 2023

If you want to prevent a span from being reported, you can:

  • Write an ObservationPredicate and filter out the Observation: this will also filter out metrics.
  • Write a SpanExportingPredicate (see: SpanIgnoringSpanExportingPredicate): this will only filter out spans and leaves metrics alone.

You can inject your own sampler but it is for a different use-case, I would go with the options above.

Here's a working example (similar to Moritz's: #34400 (comment)) that filters out Observations for actuator on the server (and on the client side): CommonActuatorConfig

I'm not sure I get the issue with subsequent spans: with actuator, how do you have any (custom healthcheck)? You can get the parent, there is a getParentObservation method.

@jonatan-ivanov jonatan-ivanov added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team status: waiting-for-triage An issue we've not yet triaged labels Mar 3, 2023
@braunsonm
Copy link
Author

@jonatan-ivanov The problem with this solution is the predicate only acts on spans. I know there is a getParentObservation method but it is always null when a predicate is used to filter out the ServerRequestObservationContext.

What you end up getting is a bunch of Spring Security traces that are disconnected (have no parent) which makes a mess when you're using a tool like tempo and trying to query for things. This also means that any custom observation you have in your Spring application still get reported but you can't see the context of why they ran (if they were initiated by a call from a path that was excluded). Yes we do register custom health checks, but this use case goes beyond just ignoring actuator.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 6, 2023
@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Mar 7, 2023

I'm not sure what you mean by "the predicate only acts on spans". The ObservationPredicate acts on Observations the SpanPredicate acts on Spans. What else should they act on other than Observations/Spans?

I know there is a getParentObservation method but it is always null when a predicate is used to filter out the ServerRequestObservationContext.

Since the ServerRequestObservationContext belongs to the Observation that belongs to the request that the server received, it has no parent, that's the first Observation. What parent should it have?

What you end up getting is a bunch of Spring Security traces

Now we are talking! :) If you don't want to see the Spring Security spans at all, you can disable them. If you only want to see them when the Observation of the server request is not filtered out, then similarly you can disable them only if they don't have a parent.

@jonatan-ivanov jonatan-ivanov added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Mar 7, 2023
@braunsonm
Copy link
Author

braunsonm commented Mar 7, 2023

I think we are misunderstanding eachother so I put together a minimum reproducable sample you can play with.

What I expect to happen: Any spans related to a call to /actuator/health should not be reported.

What actually happens: Any custom observations, spring security observation, or datasource observations etc are still reported but are now disconnected.

From what I gathered from your reply, you are working off the assumption that something like this should work:

@Configuration
public class TestConfiguration {
    @Bean
    ObservationPredicate ignoreActuator() {
        return (s, context) -> ignoreActuatorRecursive(context);
    };

    public boolean ignoreActuatorRecursive(Observation.ContextView context) {
        if (context instanceof ServerRequestObservationContext serverRequestObservationContext) {
            HttpServletRequest carrier = serverRequestObservationContext.getCarrier();
            return !carrier.getServletPath().startsWith("/actuator");
        }

        if (context.getParentObservation() != null) {  // <----- getParentObservation() is always null
            return ignoreActuatorRecursive(context.getParentObservation().getContextView());
        }

        return true;
    }
}

This results in the following being reported:
image

This is incorrect behaviour as it creates disconnected spans and doesn't actually result in any meaningful reduction in data reported to the tracing backend. The configuration doesn't work for a few reasons:

  1. There is never a parent observation on these observations
  2. Spring security does not have http.url attributes to match on (and they shouldn't)
  3. Any custom observations you might have in your application, like datasources etc will also be reported here unless you add attributes to match the URL on to every single observation (some of which you do not control)

Does this help you at all? This application is running with only that configuration, and the following in the pom:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
	<modelVersion>4.0.0</modelVersion>
	<parent>
		<groupId>org.springframework.boot</groupId>
		<artifactId>spring-boot-starter-parent</artifactId>
		<version>3.0.3</version>
		<relativePath/> <!-- lookup parent from repository -->
	</parent>
	<groupId>com.example</groupId>
	<artifactId>demo</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	<name>demo</name>
	<description>Demo project for Spring Boot</description>
	<properties>
		<java.version>17</java.version>
	</properties>
	<dependencies>
		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-actuator</artifactId>
		</dependency>
		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-web</artifactId>
		</dependency>
		<dependency>
			<groupId>org.springframework.boot</groupId>
			<artifactId>spring-boot-starter-security</artifactId>
		</dependency>
		<dependency>
			<groupId>io.micrometer</groupId>
			<artifactId>micrometer-observation</artifactId>
		</dependency>
		<dependency>
			<groupId>io.micrometer</groupId>
			<artifactId>micrometer-tracing-bridge-otel</artifactId>
		</dependency>
		<dependency>
			<groupId>io.opentelemetry</groupId>
			<artifactId>opentelemetry-exporter-zipkin</artifactId>
		</dependency>
		<dependency>
			<groupId>io.micrometer</groupId>
			<artifactId>micrometer-registry-prometheus</artifactId>
			<scope>runtime</scope>
		</dependency>
	</dependencies>

	<build>
		<plugins>
			<plugin>
				<groupId>org.springframework.boot</groupId>
				<artifactId>spring-boot-maven-plugin</artifactId>
			</plugin>
		</plugins>
	</build>

</project>

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 7, 2023
@mhalbritter
Copy link
Contributor

A sample definitely helps, thanks. Can you please share it with us?

@braunsonm
Copy link
Author

@mhalbritter I did, it has no other changes than the configuration class above. I also provided all the dependencies used. If that isn't enough and you're looking for more let me know..

@jonatan-ivanov
Copy link
Member

I guess what @mhalbritter is asking is a project we can clone, import to the IDE and debug. What we see is a code snippet above not a sample project.

I haven't tried to set-up a sample project for this but there is one thing that confuses me, in your snippet I see this:

if (context.getParentObservation() != null) {  // <----- THIS IS NEVER TRUE

But then you say:

There is never a parent observation on these observations

So is there a parent or not. If there is a parent, what is it?

@braunsonm
Copy link
Author

braunsonm commented Mar 7, 2023

There is never a parent. context.getParentObservation != null is always false (ie it is always null). I will modify my comment in that code snippet to make it more clear.
I don't have a sample project to load for you, but I provided all the pom and code you would need to run it locally.

@jonatan-ivanov
Copy link
Member

So the security spans should have a parent. That was my assumption when I tried to provide you a workaround: if they always have a parent, the only scenario when they don't is when their parent is ignored so you can ignore them too so you can do something like:

if (context..getName().startsWith("spring.security.") && context.getParentObservation() == null) {
    return false; // assuming parent is also ignored
}

If this assumption does not hold, we need to fix instrumentation in Spring Security.
Let us discuss this internally but could you please try another workaround? Could you please try to use a SpanFilter and set sampled to false for actuator spans?

@braunsonm
Copy link
Author

braunsonm commented Mar 7, 2023

Your assumption does not hold. ALL Observations (ignored or not) do not have a parent set.

The function signature of SpanFilter's do not allow modifying a sampled state, just adding/removing data from the span as they operate on FinishedSpan.

https://github.com/micrometer-metrics/tracing/blob/e76ef15fd01df8d8711c7f3b9ee9003b1bae224f/micrometer-tracing/src/main/java/io/micrometer/tracing/exporter/FinishedSpan.java#L35

Instead I tried using a SpanExportingPredicate but it is never called. Types left in to make it clear.

    @Bean
    SpanExportingPredicate spanExportingPredicate() {
        return new SpanExportingPredicate() {
            @Override
            public boolean isExportable(FinishedSpan span) {
                return span.getTags().get("http.url") == null || !span.getTags().get("http.url").startsWith("/actuator");
            }
        };
    }

If it did get called I expect it would have the same problem as the ObservationPredicate

@mhalbritter
Copy link
Contributor

mhalbritter commented Mar 8, 2023

Sigh. Here's the sample: sb34400.zip. Instructions to run are in the readme.

@jonatan-ivanov
Copy link
Member

@mhalbritter Thank You!

@braunsonm

Your assumption does not hold. ALL Observations (ignored or not) do not have a parent set.

I'm sorry I missed that the parent is set after the predicate is tested so the parent will be there but not at the time where you need it in the predicate, I opened an issue to improve this: micrometer-metrics/micrometer#3678

I think not being able to see what request the Spring Security Observations belong to is an issue, I created one for that too: spring-projects/spring-security#12854
Fyi: this does not need any key-values or tags to be set it is enough if the Context holds this information since ObservationPredicate can access it.

Instead I tried using a SpanExportingPredicate but it is never called.

I missed that SpanExportingPredicate was added in 3.1.0-M1: #34002 If you want to use it, either you need to use 3.1.x or you can do the same what you see in the pull request (you might need to override (an)other bean(s)).

I think I can give you a not so simple workaround:

// 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");
}

This workaround will not be necessary once the Spring Security issue above is fixed.
What do you think about closing this issue in Boot? I think there is not much we can do here.

@jonatan-ivanov jonatan-ivanov added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Mar 8, 2023
@braunsonm
Copy link
Author

Fine with me, in particular I do not think having http.url added to specific observations is the correct approach here, as I mentioned custom observations or ones provided by libraries would need to also have an attribute added to them that might have a leaky abstraction.

The micrometer issue you opened I think will solve this when it is completed.

Thanks for your thorough look into this issue.

@jonatan-ivanov
Copy link
Member

I do not think having http.url added to specific observations is the correct approach here

That's why I commented that you can use whatever data you want (e.g.: ignoreSpan=true), please feel free to use your own key-value, you "just" need to maintain the "contract" between the two components.

@jonatan-ivanov jonatan-ivanov closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2023
@jonatan-ivanov jonatan-ivanov added for: external-project For an external project and not something we can fix and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 8, 2023
@sibethencourt
Copy link

Hi @jonatan-ivanov I've tried to apply your fix by adding the config from #34002 but turns out that the SpanProcessor does not have the @ConditionalOnMissingBean annotation so I cannot replace it, unless there is some way that I don't know of?

It seems to be precisely the only bean that doesn't have the annotation, is that expected or could be a miss? https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfiguration.java#L127

@jonatan-ivanov
Copy link
Member

@sibethencourt If you check where the SpanProcessor is used, you will find SdkTracerProvider. So you might need to create a SdkTracerProvider as well depending on your situation.

This seems intentional to me, if you check how ObjectProvider<SpanProcessor> is used:

spanProcessors.orderedStream().forEach(builder::addSpanProcessor);

So it supports multiple processors that are additional.

@sibethencourt
Copy link

Hi Jonatan,

thanks for the reply. Yes I managed to create a second span processor with support for SpanExportingPredicate but since the original is also there and I can't disable it, both were sending traces so at the end even if one managed to filter traces successfully the original was sending it anyway.

I think if I create a new SdkTracerProvider will work I'll give it a try, thank you 👍

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 theme: observability Issues related to observability
Projects
None yet
Development

No branches or pull requests

6 participants