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

includeTraceContext is always set to true despite override #23

Closed rtbenfield closed 1 year ago

rtbenfield commented 1 year ago

It seems that despite overriding fetch.includeTraceContext in the config function, the traceparent header is always set on outgoing fetch calls.

I tracked this down to this line in parseConfig that uses || true rather than ?? true to coalesce the value. It also appears that retrieving this value here in instrumentFetcher does not invoke the function, resulting in the value always being truthy for function use cases.

I'm happy to raise a PR to address these two spots if you're open to contributions πŸ™‚

Thanks for publishing this library. The automatic instrumentation of Cloudflare bindings is well done πŸ₯‡

evanderkoogh commented 1 year ago

I would love PRs. I am really looking to make this a project that can work even when I don’t have as much time to work on.

It is too important for the ecosystem to let things slip πŸ˜‰

rtbenfield commented 1 year ago

Awesome! I can also share my use case here in case anyone comes across this issue with a similar one.

We have a Worker that receives an HTTP request generated by our users. That HTTP request may have a traceparent header from their app's instrumentation. We then do some work with that request and forward it to another system, but need to preserve the original traceparent value that we received. We use otel-cf-workers to capture traces from within our Worker, but need to essentially disable the context tracking from external requests in or out. The automatic instrumentation is currently overriding the traceparent of the outbound request.

With #24, we will be able to either disable the outbound trace context completely or intelligently with a snippet like:

const config: ResolveConfigFn = (env: Env, trigger) => {
  return {
    exporter: {
      // ...
    },
    fetch: {
      includeTraceContext(request) {
        // disable context propagation if the request has an existing traceparent
        return !request.headers.has("traceparent");
      }
    },
    service: {
      // ...
    },
  }
}
evanderkoogh commented 1 year ago

Thanks for the PR.. that is greatly appreciated :)