search

evanderkoogh / otel-cf-workers

An OpenTelemetry compatible library for instrumenting and exporting traces for Cloudflare Workers
BSD 3-Clause "New" or "Revised" License
239 stars 50 forks source link

option to skip context propagation for incoming requests #26

Closed rtbenfield closed 1 year ago

rtbenfield commented 1 year ago

Description

We have a Worker that receives HTTP requests from our users' code which may be instrumented with their own OpenTelemetry tracing. In this scenario, we will receive the W3C Trace Context headers with traces that we do not own. It would be great if we could opt-out of the inbound context propagation that done here in executeFetchHandler. We can currently opt-out of the outbound fetch propagation with includeTraceContext, but not inbound.

Workarounds

This could be worked around by adding another fetch around the instrument(handler, config) that deletes incoming trace headers before their reach the instrumentation. This is a bit unintuitive though.

Proposal

I could envision this being a similar API surface to includeTraceContext that would allow for using a boolean or function that returns a boolean. That way it could be based on particular characteristics of the incoming request. For example, a Worker may be receiving requests from other internal services and from external services and may propagate context from internal services only.

I am happy to write a bit more about this idea or raise a PR. I wanted to first check if this is addressed in other ways or if it is something that would be a good addition to the configuration options.

evanderkoogh commented 1 year ago

Yup.. that was always in the plan to support. I did have something like that in the previous library..

It is a matter of adding a configuration option that is checked in https://github.com/evanderkoogh/otel-cf-workers/blob/b09bef6378eeace741cc8692c8d56fad478aa162/src/instrumentation/fetch.ts#L102

Might need to have a think about exactly how we would structure the configuration for it. Something like a top level handlers, with a fetch and then acceptIncomingTraceContext?

rtbenfield commented 1 year ago

That configuration makes sense to me. That would be fitting for additional configuration options in the future too.

Would it make sense to name the property acceptTraceContext (omitting "Incoming") for consistency with includeTraceContext? I don't personally find it ambiguous.