Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(otlp-exporter-base)!: use transport interface in web exporters #4895

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Aug 1, 2024

Which problem is this PR solving?

This PR applies the pattern from #4743 to the browser exporters. The first commit in this draft is #4889, as existing tests rely on fixing this behavior first.

This PR makes it so that retry logic is shared across both Node.js and Browser, and by implementing the IExporterTransport interface for sendBeacon and XHR, moves us closer to combining all base classes.

With this change, you'll also notice that the code from all OTLPExporterNodeBase, OTLPExporterBrowserBase, and OTLPGRPCExporterBase all start looking very similar to each other, only differing in their constructor. In a follow-up I'll tackle configuration, which will allow us to combine all three into a shared base. See #4415 for how this will look like.

Breaking changes:

  • (internal) removes the export for sendWithXhr()
  • (user-facing) protected headers property was intended for internal use has been removed from all exporters

Once configuration is individually testable, we can reduce the existing tests for the exporters to higher-level integration tests as opposed to the very deep integration/unit tests that assert the state of the now individually tested sub components.

Part of #4116
Requires #4889

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Adapted existing tests
  • Manual tests using the opentelemetry-web example

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (ecc88a3) to head (aef7d15).
Report is 71 commits behind head on main.

Files Patch % Lines
...xporter-base/src/platform/browser/xhr-transport.ts 93.10% 2 Missing ⚠️
...base/src/platform/browser/send-beacon-transport.ts 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4895      +/-   ##
==========================================
+ Coverage   91.04%   92.16%   +1.11%     
==========================================
  Files          89      158      +69     
  Lines        1954     3471    +1517     
  Branches      416      761     +345     
==========================================
+ Hits         1779     3199    +1420     
- Misses        175      272      +97     
Files Coverage Δ
...base/src/platform/browser/send-beacon-transport.ts 92.30% <92.30%> (ø)
...xporter-base/src/platform/browser/xhr-transport.ts 93.10% <93.10%> (ø)

... and 67 files with indirect coverage changes

@pichlermarc pichlermarc force-pushed the feat/browser-exporter-transport-2 branch from c365399 to aef7d15 Compare August 9, 2024 07:41
@pichlermarc pichlermarc marked this pull request as ready for review August 9, 2024 07:55
@pichlermarc pichlermarc added this pull request to the merge queue Aug 21, 2024
Merged via the queue into open-telemetry:main with commit f2a6bcc Aug 21, 2024
19 checks passed
@pichlermarc pichlermarc deleted the feat/browser-exporter-transport-2 branch August 21, 2024 10:36
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
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