refactor: remove "export *" in favor of explicit named exports #4880

robbkidd · 2024-07-25T22:55:51Z

Which problem is this PR solving?

In service of Change / Ban all usages of export * from ... so that all exports (and imports) are explicit #4186

Short description of the changes

Add the eslint rule to the api package's lint config.
Make the rule pass.
- Exports _globalThis explicitly.
add the rule to base eslint
update all packages' exports to make explicit what is currently implicit
identify which-if-any packages should be excluded from this PR to avoid conflicts with current work-in-progress
- semconv - yep Use weaver to generate latest semconv 1.27 #4690
- otlp-exporters - yep
- NodeSDK - yep
remove the changes from those packages and set eslint "no-restricted-syntax": ["warn", "ExportAllDeclaration"] for them
add list of packages export *'ing untouched by this PR to Change / Ban all usages of export * from ... so that all exports (and imports) are explicit #4186 for tracking
- Done.

export * untouched by this PR

- [ ] experimental/packages/exporter-logs-otlp-grpc/src/index.ts
- [ ] experimental/packages/exporter-trace-otlp-grpc/src/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/platform/browser/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/platform/index.ts
- [ ] experimental/packages/exporter-trace-otlp-http/src/platform/node/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/src/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/browser/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/platform/node/index.ts
- [ ] experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/index.ts
- [ ] experimental/packages/opentelemetry-sdk-node/src/index.ts
- [ ] experimental/packages/otlp-exporter-base/src/index.ts
- [ ] packages/opentelemetry-semantic-conventions/src/index.ts
- [ ] packages/opentelemetry-semantic-conventions/src/resource/index.ts
- [ ] packages/opentelemetry-semantic-conventions/src/trace/index.ts

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

a very small test added to integration tests for baggageUtils; not comprehensive
ad-hoc test by @trentm confirmed exports match before and after these changes

Checklist:

Followed the style guidelines of this project
Unit tests have been added
- In as much as this is unit-testable, yes.
Documentation has been updated
- changelogs and doc comments; no interfaces changed

codecov · 2024-07-25T23:09:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.64%. Comparing base (ecc88a3) to head (919854d).
Report is 59 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4880      +/-   ##
==========================================
+ Coverage   91.04%   93.64%   +2.60%     
==========================================
  Files          89      269     +180     
  Lines        1954     6688    +4734     
  Branches      416     1325     +909     
==========================================
+ Hits         1779     6263    +4484     
- Misses        175      425     +250

see 219 files with indirect coverage changes

Remove special case eslint rule from API.

Sanity check before the rewrite of export * as nameSpace to explicit exports under a nameSpace. Co-authored-by: Jamie Danielson <[email protected]>

robbkidd · 2024-07-30T22:47:00Z

I've gotta catch this branch up to main. CI build errors are caused by the now explicit named exports added from a time before #4743 removed some functions.

pichlermarc · 2024-07-31T07:48:46Z

@robbkidd thanks for working on this.

I'm currently working on another large change for the OTLP exporters, so you can omit them in this PR for now. The exporters currently have differently named exports for web and node. This causes trouble when we're trying to convert them to explicitly named exports and also introduces some bugs along the way.

The fix I'm working on will ensure that the exports for web and node are the same so that we don't run into that issue anymore.

robbkidd · 2024-07-31T16:05:09Z

packages/opentelemetry-semantic-conventions/src/resource/index.ts

@@ -13,4 +13,144 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
-export * from './SemanticResourceAttributes';
+export {


@dyladan Should I exclude this package from the export cleanup because #4690? 😅

I updated this branch to not change the semantic conventions export-all yet. Maybe that can happen as a part of #4690 and the code-generation?

@robbkidd yes ignore semconv. The semconv package might end up keeping the export * for a couple reasons:

it simplifies code generation

it is only a "single level" of export, and most tree shakers can actually handle that.

To expand on (2), the issue with import * is not that it is always bad, but that if you have some file that exports *, then another file re-exports that file, then maybe that happens with another file, tree shakers have a very hard time. Every tree shaker has a maximum number of levels that it tracks (many, such as eslint, are just 1 level). Additionally, because the semconv package only exports constants, there is no fear of reaching depth limits of tree shakers in other ways (circular references, deep objects, etc).

There are two levels of re-export, at least in the current semconv package:

♻️ src/index.ts

♻️ src/resource/index.ts

⬆️ src/resource/SemanticResourceAttributes.ts (the real consts)

♻️ src/trace/index.ts

⬆️ src/resource/SemanticAttributes.ts (the real consts)

I see that the proposed new structure for semconv will have only one-level, so maybe it's fine to keep the two levels here in the deprecated paths?

Mostly scripted. Thanks, Trent! Re-exports under namespaces and re-exports from another package were done by hand. We have intentionally not tackled NodeSDK yet. Awaiting feedback on this approach so far before updating that package. OTLP exporters omitted from this pass because work is in progress on them in other branches. Co-authored-by: Trent Mick <[email protected]> Co-authored-by: Jamie Danielson <[email protected]>

Co-authored-by: Jamie Danielson <[email protected]>

Updated the comment to remove the TODO because there is no expected followup. SemConv will use `export *` of the generated constants.

JamieDanielson · 2024-08-02T18:03:20Z

TODO

I think we can mark this TODO as complete, I've added as a comment to the issue.

trentm

Thanks for doing this.
This change was pleasantly fewer lines of change than I expected this.

I have one Q below about an added devDep.
This needs a changelog entry.

To somewhat test this I wrote a script to check that the CommonJS exported names from every built .js file is the same before and after:
https://gist.github.com/trentm/e93843753d7bb2b049566fbddb4c1fc4
And they are the same.

trentm · 2024-08-02T19:55:10Z

integration-tests/api/package.json

    "@types/mocha": "10.0.7",
    "@types/node": "18.6.5",
    "codecov": "3.8.3",
    "cross-var": "1.1.0",
    "lerna": "6.6.2",
    "mocha": "10.0.0",
-    "nyc": "15.1.0"
+    "nyc": "15.1.0",
+    "ts-node": "^10.9.2"


Do you know why this ts-node devDep was added?

🤔 I wonder if we left that in when trying out some other integration tests. Doesn't look like we ended up using it for anything, should be okay to drop.

I think the way we've set it up now will always require ts-node when we're using mocha, even if we're just feeding it JavaScript code (see ./.mocharc.yml, which is the closes mocha config file).

So I think we'd actually need to add this ts-node dependency here.

Do you know why this ts-node devDep was added?

… I did last week.

I removed ts-node from package.json and the root package-lock.json, rm'd root ./node_modules, ran npm run reset, then this integration test cd integration-tests/api && npm run test, and the tests ran and passed. So … I can't explain the dev need for ts-node any more.

Update: I removed ts-node from the devDeps and the test suite seems fine.

Regarding changelog entries: should supply multiple entries to a CHANGELOG (one entry for every package touched in the closest parent CHANGELOG)? I would model each entry's content on this past exports refactoring.

Changelogs!

❯ find ./ -type d -name node_modules -prune -o -name CHANGELOG.md -print | sort .//CHANGELOG.md .//api/CHANGELOG.md .//experimental/CHANGELOG.md .//experimental/packages/otlp-transformer/protos/CHANGELOG.md .//scripts/semconv/opentelemetry-specification/CHANGELOG.md

I think we should skip the changelog in the otlp-transformer package and the scripts directory, so we'd maybe want only in these:

.//CHANGELOG.md

.//api/CHANGELOG.md

.//experimental/CHANGELOG.md

Changelog entries added in fbe156f. I opted to list each package individually in sub-bullets because I didn't see prior art for very long lists of updated packages. I'm happy to adjust that content—flatten to a single bullet with a comma-separated list, put the long list in refactor(...):, remove the list—however y'all see fit.

Update: I removed ts-node from the devDeps and the test suite seems fine.

I understand the ts-node dep now. Marc's comment is correct. In #4840 we switched from using ts-mocha to just using mocha directly with a config file (https://github.com/open-telemetry/opentelemetry-js/blob/main/.mocharc.yml) that uses ts-node (to handle the compilation of TS to JS). That means that technically any package.json file with a "mocha" devDep should also have a "ts-node" devDep. Because we are using npm workspaces with a shared top-level node_modules/... folder, any single package.json have a ts-node devDep that gets installed at the top-level saves us. And there are three:

% rg ts-node -g package.json packages/opentelemetry-semantic-conventions/package.json 90: "ts-node": "10.9.2", experimental/examples/events/package.json 6: "start": "ts-node index.ts" 18: "ts-node": "^10.9.1" experimental/examples/logs/package.json 6: "start": "ts-node index.ts", 16: "ts-node": "^10.9.1"

So, technically adding that devDep here was a good thing, but this is hardly the only transgressor. There are 3 package.json's with the "ts-node" dep and 44 with the "mocha" dep:

% rg '"mocha":' -g package.json | wc -l 44

pichlermarc

Just one nit about the location of the @opentelemetry/core integration test.
Thank you for working on this, looks great 🙂

integration-tests/api/test/api-entries.test.js

api-entries is about api; core-entries is about core. Both are still under ./integration-tests/api, but maybe that's OK?

Maybe we don't need ts-node here after all?

JamieDanielson · 2024-08-07T19:13:59Z

I think everything has been updated to match feedback, with the last missing piece being the changelog(s) entry. I mention in this comment that we should probably have a changelog in root, api, and experimental. This seems to match the note in root changelog as well, which makes me wonder whether those other changelogs should still exist or be removed. That is not something to address here though.

# CHANGELOG

All notable changes to this project will be documented in this file.

For API changes, see the [API CHANGELOG](api/CHANGELOG.md).
For experimental package changes, see the [experimental CHANGELOG](experimental/CHANGELOG.md).

…telemetry#4880) Co-authored-by: Jamie Danielson <[email protected]> Co-authored-by: Trent Mick <[email protected]> Co-authored-by: Marc Pichler <[email protected]>

robbkidd added 2 commits July 25, 2024 18:49

add no export-star rule to api's eslint

0ba7e65

no more export-star, only specifics

919854d

robbkidd and others added 2 commits July 30, 2024 17:59

declare the rule at the root

e1aab56

Remove special case eslint rule from API.

add baggageUtils check to integration tests

dd73e23

Sanity check before the rewrite of export * as nameSpace to explicit exports under a nameSpace. Co-authored-by: Jamie Danielson <[email protected]>

robbkidd commented Jul 31, 2024

View reviewed changes

robbkidd changed the title ~~chore(api): 🚧 remove all "export *" from package~~ chore: 🚧 remove all "export *" from package Jul 31, 2024

robbkidd changed the title ~~chore: 🚧 remove all "export *" from package~~ chore: 🚧 remove all "export *" from packages Jul 31, 2024

robbkidd force-pushed the api.no-export-star branch from 7c927b0 to 040dd47 Compare July 31, 2024 22:28

robbkidd and others added 2 commits July 31, 2024 18:37

dial eslint down to warn on exports being worked separately

6b46bd8

Co-authored-by: Jamie Danielson <[email protected]>

robbkidd force-pushed the api.no-export-star branch from 040dd47 to 6b46bd8 Compare July 31, 2024 22:40

Merge branch 'main' into api.no-export-star

8a4b892

robbkidd marked this pull request as ready for review July 31, 2024 22:58

robbkidd requested a review from a team July 31, 2024 22:58

robbkidd changed the title ~~chore: 🚧 remove all "export *" from packages~~ chore: remove all "export *" from packages Jul 31, 2024

robbkidd changed the title ~~chore: remove all "export *" from packages~~ chore: remove "export *" from most packages Jul 31, 2024

disable warning about 'export *' in semconv

0fe69ad

Updated the comment to remove the TODO because there is no expected followup. SemConv will use `export *` of the generated constants.

JamieDanielson mentioned this pull request Aug 2, 2024

Change / Ban all usages of export * from ... so that all exports (and imports) are explicit #4186

Open

trentm reviewed Aug 2, 2024

View reviewed changes

pichlermarc approved these changes Aug 5, 2024

View reviewed changes

integration-tests/api/test/api-entries.test.js Outdated Show resolved Hide resolved

pichlermarc added the target:next-minor-release This PR is in scope for the next minor release (`main` branch) label Aug 7, 2024

robbkidd added 3 commits August 7, 2024 12:12

move assertions about core to separate test file

f0124ea

api-entries is about api; core-entries is about core. Both are still under ./integration-tests/api, but maybe that's OK?

Merge remote-tracking branch 'upstream/main' into api.no-export-star

fbe20c5

remove ts-node from integration test deps

9c0b671

Maybe we don't need ts-node here after all?

JamieDanielson approved these changes Aug 7, 2024

View reviewed changes

saidsef mentioned this pull request Sep 18, 2024

[Snyk] Upgrade: , , , , saidsef/tracing-node#195

Merged

micahg mentioned this pull request Sep 18, 2024

[Snyk] Upgrade: , , micahg/tbltp#288

Open

danqueb mentioned this pull request Sep 19, 2024

[Snyk] Upgrade: , , danqueb/opentelemetry.io#2

Open

This was referenced Sep 19, 2024

[Snyk] Upgrade: , , , , pwalsh-snyk/microservices-app#8

Open

[Snyk] Upgrade: , , , , pwalsh-snyk/microservices-app#9

Open

WontonSam mentioned this pull request Sep 19, 2024

[Snyk] Upgrade: , , WontonSam/opentelemetry.io#39

Open

zx8086 mentioned this pull request Sep 19, 2024

[Snyk] Upgrade: , , , , , , , zx8086/capella-document-search#61

Merged

WontonSam mentioned this pull request Sep 20, 2024

[Snyk] Upgrade: , , , cspell, gulp, hugo-extended, markdownlint, postcss-cli, textlint, textlint-rule-terminology WontonSam/opentelemetry.io#40

Open

pwalsh-snyk mentioned this pull request Sep 24, 2024

[Snyk] Upgrade @opentelemetry/semantic-conventions from 1.25.1 to 1.27.0 pwalsh-snyk/microservices-app#10

Open

This was referenced Sep 24, 2024

[Snyk] Upgrade @opentelemetry/instrumentation from 0.29.2 to 0.53.0 danqueb/opentelemetry.io#3

Open

[Snyk] Upgrade @opentelemetry/exporter-trace-otlp-http from 0.29.2 to 0.53.0 danqueb/opentelemetry.io#4

Open

This was referenced Sep 27, 2024

[Snyk] Upgrade @opentelemetry/exporter-trace-otlp-http from 0.41.2 to 0.53.0 WontonSam/opentelemetry.io#43

Open

[Snyk] Upgrade @opentelemetry/instrumentation from 0.41.2 to 0.53.0 WontonSam/opentelemetry.io#44

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove "export *" in favor of explicit named exports #4880

refactor: remove "export *" in favor of explicit named exports #4880

robbkidd commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024

robbkidd commented Jul 30, 2024

pichlermarc commented Jul 31, 2024

robbkidd Jul 31, 2024

robbkidd Jul 31, 2024

dyladan Aug 1, 2024

robbkidd Aug 1, 2024

JamieDanielson commented Aug 2, 2024

trentm left a comment •

edited by pichlermarc

Loading

trentm Aug 2, 2024

JamieDanielson Aug 2, 2024

pichlermarc Aug 5, 2024

robbkidd Aug 7, 2024 •

edited

Loading

robbkidd Aug 7, 2024 •

edited

Loading

JamieDanielson Aug 7, 2024

robbkidd Aug 7, 2024

trentm Aug 8, 2024 •

edited

Loading

pichlermarc left a comment

JamieDanielson commented Aug 7, 2024

refactor: remove "export *" in favor of explicit named exports #4880

refactor: remove "export *" in favor of explicit named exports #4880

Conversation

robbkidd commented Jul 25, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jul 25, 2024

Codecov Report

robbkidd commented Jul 30, 2024

pichlermarc commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieDanielson commented Aug 2, 2024

trentm left a comment • edited by pichlermarc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbkidd Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

robbkidd Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentm Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

JamieDanielson commented Aug 7, 2024

robbkidd commented Jul 25, 2024 •

edited

Loading

trentm left a comment •

edited by pichlermarc

Loading

robbkidd Aug 7, 2024 •

edited

Loading

robbkidd Aug 7, 2024 •

edited

Loading

trentm Aug 8, 2024 •

edited

Loading