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

Allow overriding per-request sampling decision when tracing OkHttp requests #1750

Open
lwasyl opened this issue Nov 28, 2023 · 2 comments
Open
Labels
enhancement New feature or request

Comments

@lwasyl
Copy link

lwasyl commented Nov 28, 2023

Are you requesting automatic instrumentation for a framework or library? Please describe.

  • Framework or library name: OkHttp
  • Library type: networking
  • Library version: 4.12.0

Is your feature request related to a problem? Please describe.
To debug a specific issue, I'd like to dynamically decide to trace all requests of a given type/endpoint. Currently I can pass traceSampler to a TracingInterceptor, but it only allows providing dynamic sampling rate.

Describe the solution you'd like
I'd like a clear API to override whether a request should be traced, regardless of the default/fallback sampling rate, something like:

TracingInterceptor(
    perRequestSampler = { request: okhttp3.Request -> 
        if (shouldSample(request)) {
             Sampler.Sample
        } else if (shouldIgnore(request)) {
             Sampler.Ignore
        } else {
            Sampler.UseDefault
        }
    }
)

Describe alternatives you've considered
Currently I'm using a custom interceptor added before the TracingInterceptor:

Interceptor { chain ->
    val request = chain.request().let { request ->
        if (forceSample(request)) {
            request.newBuilder()
                .addHeader("x-datadog-sampling-priority", "${PrioritySampling.USER_KEEP}")
                .build()
        } else {
            request
        }
    }

    chain.proceed(request)
},

This approach seems to work but:

  • it requires adding the interceptor in the right place (before the TracingInterceptor) or else it will silently not work
  • I couldn't find any documentation for x-datadog-sampling-priority header. There are two constants in the code with this string but both are internal, suggesting it shouldn't be used by the consumers. I also couldn't find any documentation suggesting that PrioritySampling constants should be used as the values for that header, so generally this feels like an implementation detail that may break with the SDK updates.

Additional context

@lwasyl lwasyl added the enhancement New feature or request label Nov 28, 2023
@lwasyl lwasyl changed the title Allow per-request sampling when tracing Allow per-request sampling when tracing OkHttp requests Nov 28, 2023
@lwasyl lwasyl changed the title Allow per-request sampling when tracing OkHttp requests Allow overriding per-request sampling decision when tracing OkHttp requests Nov 28, 2023
@xgouchet
Copy link
Member

Hi @lwasyl , and thanks for opening this Feature Request,

it's indeed an interesting use case that we don't support for now. We will add this to our backlog and see if we can implement it in the future.

@xgouchet
Copy link
Member

Regarding the x-datadog-sampling-priority header, even though it is marked as internal, it does work as a valid workaround, and there's no issue in using that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants