TraceExporter for `fetch`-only environment - Githubissues

open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client

https://opentelemetry.io

Apache License 2.0

2.69k stars 779 forks source link

TraceExporter for `fetch`-only environment #4631

Open lacolaco opened 5 months ago

lacolaco commented 5 months ago

NB: Before opening a feature request against this repo, consider whether the feature should/could be implemented in the other OpenTelemetry client libraries. If so, please open an issue on opentelemetry-specification first.

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

Currently, built-in TraceExporter implementation for browser depends on XMLHttpRequest or sendBeacon. These can't be adopted for fetch-only environments like WebWorker, ServiceWorker, or some edge workers (e.g. Cloudflare workers).

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts#L59

Once we accept fetch as an HTTP request sender, it can also be used in Node.js environments.

Describe the solution you'd like

Option 1: Add "fetch mode" to OTLPExporterBrowserBase
Option 2: Add a different class like OTLPExporterFetch

Describe alternatives you've considered

Additional context

3413 This issue also mentions fetch but only about instrumentation.

david-luna commented 5 months ago

Hi @lacolaco

I would like to understand better your use case. Do you want to instrument a WebWorker/ServiceWorker that runs on the Browser? If so, do you also want to instrument the UI thread and correlation between traces form the UI and the worker?

lacolaco commented 5 months ago

@david-luna I have two use cases; ServiceWorker on the browser and Cloudflare Workers. ServiceWorker needs to be able to propagate traces from UI threads to and from the backend server. This is equivalent to a proxy server from an overall perspective. The Cloudflare Worker is simply instrumentation as a back-end server.

david-luna commented 5 months ago

I'm not familiar with Cloudfare Workers but regarding Service Workers I think sending traces with fetch is not enough to have the proper instrumentation.

At least instrumentation of FetchEvent needs to be implemented so context is properly propagated when creating spans within the SW. Also I think there might be necesary to review core and web-sdk packages to make sure they are not using APIs not available in workers.

lacolaco commented 5 months ago

To clarify in case there is some misunderstanding, I am referring to the implementation of the exporter using the fetch api, not instrumentation for the fetch api.

david-luna commented 5 months ago

okay, so I guess you will do manual instrumentation of the incoming requests to the Service Worker to keep the trace context from the UI. I'll share today with the JavaScript SIG or maybe you can join and discuss this topic.

lacolaco commented 5 months ago

Thank you for the escalating!

david-luna commented 5 months ago

Checking on the agenda notes there is a discussion about this and also a PR. I'm adding the links here to complete the context. Discussion: https://github.com/open-telemetry/opentelemetry-js/issues/3845 PR: https://github.com/open-telemetry/opentelemetry-js/pull/3542

david-luna commented 5 months ago

@lacolaco

We did discuss adding this feature and definitely is a feature we want in OTEL. PR #3542 it's quite complex but it will get simpler once #4542 lands into the main branch. So the plan would be:

merge #4542
update #3542 to use the new exporter API
review and merge #3542
wait for the next release

lacolaco commented 5 months ago

@david-luna Sounds nice! Thank you all for the great job!

pichlermarc commented 5 months ago

@lacolaco

We did discuss adding this feature and definitely is a feature we want in OTEL. PR #3542 it's quite complex but it will get simpler once #4542 lands into the main branch. So the plan would be:

* merge [feat!: move serialization to `@opentelemetry/otlp-transformer` #4542](https://github.com/open-telemetry/opentelemetry-js/pull/4542)

* update [feat(otlp-exporter-base): Add fetch sender for ServiceWorker environment #3542](https://github.com/open-telemetry/opentelemetry-js/pull/3542) to use the new exporter API

* review and merge [feat(otlp-exporter-base): Add fetch sender for ServiceWorker environment #3542](https://github.com/open-telemetry/opentelemetry-js/pull/3542)

* wait for the next release

Thank you @david-luna for summarizing this and bringing it up in the SIG.

To expand on what David said, there are a few more steps to be able to merge #3542 - we'll need to re-structure how we handle transports in the OTLP exporters (see https://github.com/open-telemetry/opentelemetry-js/issues/4116 for a more in-depth reasoning on why this is necessary). I have a draft for this at #4415, but applying that'll be a multi-step process as we try to avoid breaking users (the exporters are quite heavily used, so we try to tread lightly with changes there).

the first of a series of changes was #4151, introducing the first step to make transports pluggable
now there's #4542 which moves some of the serialization code out to @opentelemtry/otlp-transformer so that transports become agnostic of what data they're sending.
after that #4581 will do the same for browser exporters
- this then opens the way for more changes from #4415 to be added in the repo, which will likely be 2 PRs again (Node.js HTTP transports (#4743) then pulling out Browser Transports (TBD), likely some more are needed to refactor tests)

After all these changes adding fetch as a transport should be as easy as implementing one Transport interface and adding it to the existing code, which will also make it a component that we can test individually.

danielwgetvim commented 3 months ago

hi

this is very much needed!

github-actions[bot] commented 2 weeks ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.