8000 fix(otlp-exporter-base)!: split node and browser config types in two by pichlermarc · Pull Request #5917 · open-telemetry/opentelemetry-js · GitHub
[go: up one dir, main page]
Skip to content

fix(otlp-exporter-base)!: split node and browser config types in two #5917

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 10, 2025
Merged

fix(otlp-exporter-base)!: split node and browser config types in two #5917

merged 6 commits into from
Sep 10, 2025

Conversation

pichlermarc
Copy link
Member
@pichlermarc pichlermarc commented Sep 8, 2025 •
edited
Loading

Which problem is this PR solving?

See #5908, browser exporters are broken due to dynamic import of node:http in the defaults. What is happening is that getHttpConfigurationDefaults() is called, which uses the httpAgentFactoryFromOptions() function to create an agent factory, which does not work in the browser.

This PR fixes this, by introducing a new type OtlpNodeHttpConfiguration along with node-specific merge and default functions, that are based on the ones for OtlpHttpConfiguration. This is a breaking change as the name of the type changes, however it gives us more flexibility in the future to have these configurations diverge further without breaking one another.

Related PR: #5719
Fixes #5908
Related #4744

Type of change

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

How Has This Been Tested?

  • Manually tested against examples/opentelemetry-web
  • Added webpack bundler test (for browser target) as well as a workflow to run these tests on each PR and main

@codecov Codecov
Copy link
codecov bot commented Sep 8, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.07%. Comparing base (b37bfa7) to head (54f736f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5917   +/-   ##
=======================================
  Coverage   95.06%   95.07%           
=======================================
  Files         307      308    +1     
  Lines        8031     8039    +8     
  Branches     1627     1627           
=======================================
+ Hits         7635     7643    +8     
  Misses        396      396
Files with missing lines Coverage Δ
.../configuration/convert-legacy-node-http-options.ts 100.00% <100.00%> (ø)
...ase/src/configuration/legacy-node-configuration.ts 100.00% <ø> (ø)
...-base/src/configuration/otlp-http-configuration.ts 100.00% <ø> (ø)
.../src/configuration/otlp-node-http-configuration.ts 100.00% <100.00%> (ø)
.../configuration/otlp-node-http-env-configuration.ts 95.83% <100.00%> (ø)
...packages/otlp-exporter-base/src/index-node-http.ts 100.00% <100.00%> (ø)
...tlp-exporter-base/src/otlp-http-export-delegate.ts 100.00% <ø> (ø)
...rter-base/src/transport/http-exporter-transport.ts 95.83% <100.00%> (ø)
...xporter-base/src/transport/http-transport-utils.ts 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pichlermarc pichlermarc marked this pull request as ready for review September 8, 2025 15:13
@pichlermarc pichlermarc requested a review from a team as a code owner September 8, 2025 15:13
@pichlermarc pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc pkg:otlp-exporter-base labels Sep 8, 2025
@pichlermarc
Copy link
Member Author

@maryliag thank you for taking a look. 🙂

Since you mentioned it here: I've been working on the follow-up test (#4744 like) in the background today - I just pushed them to this PR. You should be able to see them failing over at this draft #5919.

The test itself is very simple (it only runs webpack and checks if the thing builds or not), but should help us catch regressions like this in the future. 🙂

@pichlermarc
Copy link
Member Author

I had planned the extra tests to be a follow-up but it turned out to be simpler than I thought. :)

@pichlermarc pichlermarc mentioned this pull request Sep 9, 2025
Merged
@pichlermarc pichlermarc requested a review from maryliag September 9, 2025 16:49
@maryliag
Copy link
Contributor
maryliag commented Sep 9, 2025

awesome! thank you for adding the test! 🚀

@pichlermarc pichlermarc added this pull request to the merge queue Sep 10, 2025
Merged via the queue into open-telemetry:main with commit 9a8c6e0 Sep 10, 2025
25 checks passed
@pichlermarc pichlermarc deleted the fix/webpack-failing branch September 10, 2025 08:51
@opentelemetrybot opentelemetrybot mentioned this pull request Sep 10, 2025
Merged
@svrnm svrnm mentioned this pull request Sep 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryliag maryliag maryliag approved these changes

Assignees
No one assigned
Labels
bug Something isn't working pkg:otlp-exporter-base priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Webpack build failure using the experimental v0.204.0 packages
2 participants
@pichlermarc @maryliag
0