feat(otlp-exporter-base): accept fetch parameter in createFetchTransport, and export createFetchTransport and createRetryingTransport#6377
Conversation
|
|
60b2f41 to
db5111b
Compare
|
@blumamir @hectorhdzg @JacksonWeber @martinkuba @maryliag @raphael-theriault-swi @svetlanabrennan Would it be possible for a review of these changes, when you get a moment? I'm also happy to open an issue for discussion or change the approach if you would prefer. Sorry for the pings by the way, the contributing guide included that I should @ the approvers on my PR. |
|
@zakcutner - my original idea was to provide an interface for people to pass their own transports to the exporters, because different transport implementations are so widely requested. My idea was that people would be able to do something like this, but I ran out of time to work on it: const exporter = createOtlpExporter({
serializer: ProtobufTraceSerializer,
transport: createMyCustomFetchTransport() // returns a custom IExporterTransport impl
});the underlying implementation is already somewhat setup for such a thing, but requires a bit more boilerplate import { createOtlpNetworkExportDelegate } from '@opentelemetry/otlp-exporter-base';
import { ProtobufTraceSerializer } from `@opentelemetry/otlp-transformer`;
const exporter: SpanExporter = new createOtlpExportDelegate({
options: { /** provide all required options accoding to type**/ },
serializer: ProtobufTraceSerializer, // or JsonTraceSerializer
transport: createMyCustomFetchTransport() /** implement this yourself, a wrapper around your custom fetch, make sure to set the content-type header to what you're sending: `application/x-protobuf` or `application/json` **/
}); /* delegate happens to match the SpanExporter type, this won't work for metrics today though, because some methods are missing */I suppose this is what you mean by
IMO this is a good enough workaround for now. Once we drop support for older Node.js versions (around July 2026), we can also finally use |
|
So my recommendation for this feature is:
|
db5111b to
bd58fd1
Compare
fetch parameter in createFetchTransport, and export createFetchTransport and createRetryingTransport
|
@pichlermarc Thank you so much for the review, and apologies for the delay getting back to you! Your suggestion makes sense to me, but I noticed that a few things are missing to achieve this workaround:
I've reduced the scope of the change to fix these three limitations, so I can replace createOtlpNetworkExportDelegate(
getSharedConfigurationDefaults(),
JsonTraceSerializer,
createRetryingTransport({
transport: createFetchTransport({
fetch: myCustomFetch,
headers: { "Content-Type": "application/json" },
}),
}),
)When you get a moment, let me know what you think 😄 |
…nsport`, and export `createFetchTransport` and `createRetryingTransport`
bd58fd1 to
653f26f
Compare
|
|
||
| export type { IExporterTransport } from './exporter-transport'; | ||
| export { createRetryingTransport } from './retrying-transport'; | ||
| export { createFetchTransport } from './transport/fetch-transport'; |
There was a problem hiding this comment.
please add FetchTransportParameters as well
| if (typeof fetchApi.__original === 'function') { | ||
| // @ts-expect-error -- fetch could be wrapped | ||
| fetchApi = fetchApi.__original; | ||
| } |
There was a problem hiding this comment.
if the user-provided fetch already has __original, it gets wiped out since this runs unconditionally. it would be good to have a test for that case also
| const fetchStub = sinon | ||
| .stub(globalThis, 'fetch') | ||
| .resolves(new Response('test response', { status: 200 })); | ||
| const customStub = sinon.stub().callsFake(fetchStub); |
There was a problem hiding this comment.
it looks like fetchStub will execute, increasing its call count. you might want to call it directly: sinon.stub().resolves(new Response('test response', { status: 200 }));
| } | ||
|
|
||
| function isFetchNetworkErrorRetryable(error: unknown): boolean { | ||
| return error instanceof TypeError && !error.cause; |
There was a problem hiding this comment.
a custom fetch function might not have the same structure as the default browser fetch, potentially creating an issue with code that does very specific checks like this. we probably want to call out this one in particular in the docs since retry is an important feature
| private _parameters: FetchTransportParameters; | ||
|
|
||
| constructor(parameters: FetchTransportParameters) { | ||
| this._parameters = parameters; |
There was a problem hiding this comment.
i recommend checking that fetch is a function so this fails early if it's not callable. if the failure occurs inside send(), it may look like the wrong kind of error
|
Of course, my comments only apply if js core team approves. |
Which problem is this PR solving?
I'm using OpenTelemetry JS in Cloudflare Workers and I need to send telemetry to a collector running in a private network via Workers VPC. Workers VPC provides a custom
fetchfunction that can route requests through private networks, but there was no way to easily pass this to the OTLP exporters.Before this change, the only way to achieve this was to create a custom version
FetchTransport, which would require duplicating much of its implementation, or to monkey patchglobalThis.fetch.Specifying a custom
fetchfunction could also be useful for other scenarios like adding request/response interceptors, using fetch polyfills, or testing with mocks.Short description of the changes
Adds a
fetchconfiguration option tocreateFetchTransport, allowing users to provide a customfetchimplementation instead of usingglobalThis.fetch.Exports
createFetchTransportandcreateRetryingTransport, so these can be used in place of anew OTLPTraceExporter().Type of change
How Has This Been Tested?
FetchTransportusing custom fetchnpm testChecklist: