Skip to content

fix(sdk-trace-base): avoid webpack CommonJS self-reference - #6982

Open
sansynx wants to merge 4 commits into
open-telemetry:mainfrom
sansynx:fix/sdk-node-webpack-commonjs-entry
Open

fix(sdk-trace-base): avoid webpack CommonJS self-reference#6982
sansynx wants to merge 4 commits into
open-telemetry:mainfrom
sansynx:fix/sdk-node-webpack-commonjs-entry

Conversation

@sansynx

@sansynx sansynx commented Aug 6, 2026

Copy link
Copy Markdown

Summary

  • retain local bindings for the sdk-trace-base default span-attribute limits before exporting them
  • add a Node/Webpack production regression fixture for the public NodeSDK CommonJS entry
  • document the fix in the stable changelog

Root cause

TypeScript emitted internal reads of exported constants as exports.*. When Webpack tree-shook an ESM named import resolved through the CommonJS main entry, it treated that as an invalid self-reference.

Validation

  • npm exec -- tsc --build packages/opentelemetry-sdk-trace-base/tsconfig.json --pretty false
  • npm --prefix packages/opentelemetry-sdk-trace-base run lint
  • npm exec -- mocha @testFiles (21 passing focused trace-base tests)
  • node test-bundle.mjs in bundler-tests/node/webpack-5
  • npm exec -- prettier --check ...
  • npm exec -- markdownlint-cli2 CHANGELOG.md

Fixes #6981

Signed-off-by: sanath <sanathrshs@gmail.com>
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 6, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: sansynx / name: sanath (4f68e1f)

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Welcome, contributor! Thank you for your contribution to opentelemetry-js.

Important reminders:

@sansynx
sansynx marked this pull request as ready for review August 6, 2026 14:31
@sansynx
sansynx requested review from a team as code owners August 6, 2026 14:31
Copilot AI lite review requested due to automatic review settings August 6, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a webpack production bundling crash triggered when an ESM named import is resolved through a CommonJS main entry, by ensuring sdk-trace-base reads span-limit defaults from local bindings (avoiding exports.* self-references after TypeScript emit and webpack tree-shaking). It also adds a bundler regression fixture and documents the change.

Changes:

  • Refactors sdk-trace-base default span-limit constants to retain local bindings before exporting.
  • Updates the Node/webpack-5 bundler fixture to resolve via main (CommonJS) and exercises @opentelemetry/sdk-node’s public NodeSDK entry.
  • Adds an Unreleased changelog entry for the fix.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/opentelemetry-sdk-trace-base/src/utility.ts Keeps default limit constants as local bindings and exports them, preventing webpack self-reference failures.
bundler-tests/node/webpack-5/webpack.config.mjs Forces resolve.mainFields: ['main'] to reproduce CommonJS-entry bundling behavior in production mode.
bundler-tests/node/webpack-5/src/index.js Imports and instantiates NodeSDK from the public entry to cover the reported regression scenario.
bundler-tests/node/webpack-5/package.json Adds the local @opentelemetry/sdk-node dependency for the new bundling fixture.
CHANGELOG.md Documents the fix in the Unreleased bug fixes section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread CHANGELOG.md Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 6, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-08-21 22:12 UTC

Review the latest changes.

Also blocked by: Merge conflicts.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@overbalance overbalance left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend creating a new test suite instead of changing this one so the old bundling behavior is still covered.

Signed-off-by: sanath <sanathrshs@gmail.com>
@sansynx

sansynx commented Aug 6, 2026

Copy link
Copy Markdown
Author

@overbalance Thanks for the suggestion, I split the NodeSDK/CommonJS regression into a new bundler-tests/node/webpack-5-sdk-node suite in 05b7b76 and restored bundler-tests/node/webpack-5 to its previous coverage

Both suites pass independently

@sansynx
sansynx requested a review from overbalance August 6, 2026 15:03
@iivvaannxx

Copy link
Copy Markdown

Hi @overbalance @sansynx, is there an ETA for getting this merged? I have several consumers of a library I own facing the build error caused by the issue when using Webpack. Thanks!

@sansynx

sansynx commented Aug 17, 2026

Copy link
Copy Markdown
Author

@iivvaannxx it's ready, just need @overbalance approval for this to get merged!

@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.09%. Comparing base (7f3e7ea) to head (05b7b76).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6982   +/-   ##
=======================================
  Coverage   95.09%   95.09%           
=======================================
  Files         409      409           
  Lines       14259    14260    +1     
  Branches     3269     3269           
=======================================
+ Hits        13559    13560    +1     
  Misses        700      700           
Files with missing lines Coverage Δ
...ckages/opentelemetry-sdk-trace-base/src/utility.ts 100.00% <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.

@overbalance overbalance left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not entirely true 🙂 I'm a Browser maintainer so I check code that affects the client. A JS core approver still needs to look.

If you would, please add a brief comment above these constants explaining the declaration/export split is intentional. Like "TS gen CJS emit rewrites internal reads of export const to exports.*, which trips webpack’s self-reference handling when the export is tree-shaken unused (#6981, webpack/webpack#21619)

And thank you for pointing this out. I suspect the project has more instances of the same issue that will need investigation.

Comment thread CHANGELOG.md Outdated

### :bug: Bug Fixes

* fix(sdk-trace-base): avoid a Webpack self-reference error in CommonJS output #6981 @sansynx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the link to match the others.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out — I’ve updated the entry to use the same linked issue format as the surrounding changelog items: #6981

Link the reported issue in the changelog and document why the local constant declarations must stay separate from their exports

Signed-off-by: sanath <sanathrshs@gmail.com>

sansynx commented Aug 18, 2026

Copy link
Copy Markdown
Author

Thanks for the follow-up — I’ve added a short note above the constants explaining why they stay local and are exported separately

I also kept the sdk-node regression coverage in its own webpack suite, so the existing bundling behavior remains covered

The focused trace-base tests pass (22 tests), along with the trace-base compile, lint and formatting checks, changelog markdown lint, and the new Node/Webpack 5 bundle check

I used an AI coding assistant to help with the edit, then reviewed the diff and ran the checks myself

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.

[sdk-node] 0.220+ CommonJS entry crashes webpack production builds

4 participants