Add new option method WithSampler

[thomasgouveia]

Is your feature request related to a problem? Please describe.

We want to use the opentelemetry-go-instrumentation to auto-instrument our Go microservices. To reduce storage costs and provide a better sampling strategy, we want something that allows us to exclude some traces when their request path (e.g. /health) matches a regular expression.

This could be very useful in environments like Kubernetes or similar where probes are regularly sent to the app.

Describe the solution you'd like

My first idea was to write a custom Go-specific sampler, RequestPathSampler, that takes a simple list of regular expressions as a parameter. For instance, when I configure my auto instrumentation, I would like to have :

export OTEL_TRACES_SAMPLER="request_path_sampler"
export OTEL_TRACES_SAMPLER_ARG="/(health|metrics)"

I would prefer to keep if possible OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG to ensure consistency with OpenTelemetry built-in samplers configuration.

When configured with the environment variables above, all traces matching the given pattern will be automatically dropped at the instrumentation level.

I dug a little into the code and saw that in instrumentation.go, we force the usage of the trace.AlwaysSample sampler. Maybe we can update a little this part of the code, in addition to adding the RequestPathSampler code to have something more modular for the sampler implementation, through a factory for instance.

I can make a PR for this proposal if needed.

Describe alternatives you've considered

Currently, I haven't found any viable workaround to drop some traces based on the path. I forked the repository into the company's GitLab to add this feature, but this is not ideal at all because I need to maintain my version of the project in addition to the feature. It would be better to have it included in the upstream repository.

Additional context

Related issues: open-telemetry/opentelemetry-java-instrumentation#1060

[pellared]

I would rather see it as a configuration to HTTP/gRPC instrumentations.

[thomasgouveia]

Hi @pellared,

Seems better. In which repository is the code for HTTP/gRPC instrumentation? I'll move the issue to the right repository.

[pellared]

@thomasgouveia It is here. There is no need to change anything.

[thomasgouveia]

Ok perfect. I have a branch on my local fork with my changes to support the filter on the request path, I can submit a PR for this and we'll see how it should be organized in the PR review if you (or another reviewer) is ok.

[pellared]

Doing a PR always helps (even if it is not going to be merged it facilities the discussions) 👍

My preference for the env var name would be something like OTEL_GO_AUTO_HTTP_INSTRUMENTATION_FILTER_PATH

[thomasgouveia]

I'll do it ASAP.

My preference for the env var name would be something like OTEL_GO_AUTO_HTTP_INSTRUMENTATION_FILTER_PATH

Do you prefer to use only this variable? What I want to say is by default, we can keep the actual sampler and if OTEL_GO_AUTO_HTTP_INSTRUMENTATION_FILTER_PATH is not empty, we can use the custom sampler instead of the default, and we configure it with the value of the environment variable?

[RonFed]

I think it's a great idea.
If we are going to support it via env variable, I think we should also add support through the instrumentation config option (similar to how it is currently with other configs). i.e something like WithPathFilter("/health")

[gh-axel-czarniak]

Hello,

We need this too on our side and this is a great idea to implement it in the base code.

[thomasgouveia]

I think it's a great idea. If we are going to support it via env variable, I think we should also add support through the instrumentation config option (similar to how it is currently with other configs). i.e something like WithPathFilter("/health")

Sure, I'll add this before opening the PR

[RonFed]

I think it's a great idea. If we are going to support it via env variable, I think we should also add support through the instrumentation config option (similar to how it is currently with other configs). i.e something like WithPathFilter("/health")

Sure, I'll add this before opening the PR

Another point to consider is https://www.w3.org/TR/trace-context/#trace-flags
Which handles the case you want to avoid collecting spans related to some span. For example, if you filter some HTTP path spans, you may still collect child spans of the one you filtered which is probably not the desired result.

[thomasgouveia]

As stated by @MrAlias in #468, we will not add a custom sampler code directly in this repository, so that the base auto instrumentation remains generic.

Instead, I'll add a new option method WithSampler, that will allow the creation of custom auto instrumentation binaries by providing custom implementations of sampler (like already did with WithTraceExporter).

I'll update the issue title in consequence.