Skip to content

Conversation

coryhollinger
Copy link

@coryhollinger coryhollinger commented Sep 2, 2025

Which problem is this PR solving?

Hi! @Adukek8 and I were digging this package to see what options are available, and saw that createOtlpFetchExportDelegate was just added in this PR but it wasn't exported. We'd like to use it once the release is out, so this PR just adds it as an export.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • I didn't test it, as it's just adding an export. Please let me know if you'd like me to do more.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added (N/A)
  • Documentation has been updated (N/A)

@coryhollinger coryhollinger requested a review from a team as a code owner September 2, 2025 22:14
Copy link

linux-foundation-easycla bot commented Sep 2, 2025

CLA Not Signed

@coryhollinger
Copy link
Author

coryhollinger commented Sep 2, 2025

On a related note, is there a reason the Node exporter is exposed but the browser exporter is not? Same for the corresponding exporters in exporter-logs-otlp-proto. Would that be worth including in this PR?

@coryhollinger
Copy link
Author

/easycla

@pichlermarc
Copy link
Member

@coryhollinger - this is mostly internal for now, hence why it's not exported.

How are you planning to use this export? 🤔
I'm planning to re-design the exporter interface using these now internal components so seeing some current real-world usage would be really helpful.

@Adukek8
Copy link

Adukek8 commented Sep 5, 2025

Hi, @pichlermarc.

We are in a situation where sending requests with custom headers is a necessity, but where it is also important to have some mechanism similar to fetch's concept of keepalive enabled so that telemetry can still be flushed when a tab is closed before the export completes.

Given:

  • xhr dies upon tab closure and doesn't have a concept of keepalive
  • sendbeacon doesn't work with headers

neither XHR nor sendBeacon seemed like good solutions.

The only viable approach that seemed to fit our criteria was to use fetch with keepalive enabled. To achieve this, we implemented an in-house fetch transport which was later used to create custom "keepalive" OTLP(Metric/Trace/Log)Exporters. Our transport ended up looking quite similar to the one instituted in the PR @coryhollinger mentioned, and has allowed us to export most telemetry which would otherwise be dropped (subject to fetch's 64KB keepalive size limitation). It also allowed us to create a custom webvitals instrumentation for our users, which required keepalive to be enabled as some of those metrics are only available right at unload (CLS and LCP).

I notice in the PR that keepalive does seem to default to true in a web setting, which means that we are probably set in terms of our needs, so we likely don't need the delegate to be exported. This is great news, as it means we'll be able to simplify our implementation after the release drops! The only potential drawback that occurs to me given this change is that, to my understanding, there do exist request body size limitations on fetch requests with keepalive enabled, so perhaps there could be situations where users might desire some sort of keepalive opt-out (or maybe options could be configurable for the transport's fetch init dict?). I'm not sure of whether this is a limitation which would necessitate modification, but I did want to call it out in case it hadn't yet been considered.

Regardless, we're excited to make contact here and would love to stay in touch regarding opportunities to contribute work back upstream. We're also super curious about how you plan to redesign the export interface, so we'd love to hear a little more about your plans there, if those have been fleshed out :)

It's also likely that my understanding of certain nuances of this PR (and OTEL in general) are not perfect, so I'd welcome any corrections or clarification.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants