chore: remove the non-publishing packages from the npm workspace #1938

trentm · 2024-02-14T16:51:09Z

Because they cause difficulties with updating @opentelemetry/* dependencies.
This is "Option 3" from #1917

Refs: #1917

summary of changes

move all plugins/node/opentelemetry-instrumentation-FOO/examples/ to examples/FOO and fix up some details to get them working.
move packages/opentelemetry-sampler-aws-xray to incubator/opentelemetry-sampler-aws-xray/
These packages are now NOT included in the monorepo workspace.
Removed npm run compile:examples.
Removed entries for these packages from release-please config and manifest.

checklist (done)

Undo "option 1" work from chore: add non-releasing packages to release-please config so their local deps get bumped #1928
Q: is "incubator/" dir a reasonable name? Or put sampler-aws-xray in "archive/"?
move plugins/node/instr-FOO/examples to top-level examples/FOO dir. (Note that this reverses the intent from an effort 1-2y ago to move from examples/ to the instrumentation dirs.)
Try to confirm that my 'update-otel-deps.js' script now works better at updating deps in this repo.
manually test each of those examples
clean up intent in examples/README.md
~~start an issue for working through modernizing or dropping examples~~
~~document expectation/procedure for maintaining examples~~

codecov · 2024-02-14T17:13:02Z

Codecov Report

Merging #1938 (6db8f7b) into main (7d5cdb0) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1938      +/-   ##
==========================================
- Coverage   91.04%   91.02%   -0.02%     
==========================================
  Files         147      146       -1     
  Lines        7530     7491      -39     
  Branches     1507     1501       -6     
==========================================
- Hits         6856     6819      -37     
+ Misses        674      672       -2

see 1 file with indirect coverage changes

pichlermarc · 2024-02-19T10:02:56Z

Q: is "incubator/" dir a reasonable name? Or put sampler-aws-xray in "archive/"?

I think so, yes :) I think it works better than archive and this should be a dir that we can easily re-name (since it's not released, there wouldn't be a lot of links pointing towards this)

move plugins/node/instr-FOO/examples to top-level examples/FOO dir. (Note that this reverses the intent from an effort 1-2y ago to move from examples/ to the instrumentation dirs.)

That's unfortunate, but I think it's fine. Up-to-date examples are important, but I'd put a higher priority on releasing on time.

trentm · 2024-02-21T05:59:33Z

Sorry for the delay. I'll get to this tomorrow (Wednesday).

Because they cause difficulties with updating `@opentelemetry/*` dependencies. This is "Option 3" from open-telemetry#1917 Refs: open-telemetry#1917

…s to release-please config) from open-telemetry#1928

… package dirs

'npm install --package-lock-only' did not accomplish this. This regen was necessary because those vestigial entries caused surprising breakage in some 'npm install --no-save ...' commands such as TAV is doing. It broke TAV tests with [email protected].

trentm · 2024-02-22T20:01:05Z

Here is/was a nice surprise (sarcasm) with npm.

With the above changes the TAV (test-all-versions) tests were failing, but just with [email protected] (not with 4.17.0 and not with 4.17.2). What the TAV tests effectively do is: npm install --no-save mongodb@VERSION; npm test. Look at what was happening with that npm install --no-save ... attempt in the instrumentation-mongodb dir:

[~/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-mongodb (git:tm-1917-option-3)]
% npm ls mongodb
[email protected] /Users/trentm/tm/opentelemetry-js-contrib2
└─┬ @opentelemetry/[email protected] -> ./plugins/node/opentelemetry-instrumentation-mongodb
  └── [email protected]

[~/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-mongodb (git:tm-1917-option-3)]
% npm install --no-save [email protected]
...

[~/tm/opentelemetry-js-contrib2/plugins/node/opentelemetry-instrumentation-mongodb (git:tm-1917-option-3)]
% npm ls mongodb
[email protected] /Users/trentm/tm/opentelemetry-js-contrib2
└─┬ @opentelemetry/[email protected] -> ./plugins/node/opentelemetry-instrumentation-mongodb
  └── [email protected]

npm install exited successfully but it did NOT install the requested mongodb version in the current dir.

It took a while to notice that while I had removed the "plugins/node/opentelemetry-instrumentation-mongodb/examples" directory, the "package-lock.json" file still had that vestigial entry. Using npm install --package-lock-only did not remove these entries.

If I manually make this change to that vestigial entry:

% git diff -U15
diff --git a/package-lock.json b/package-lock.json
index 89c3fa77..94e12811 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -35060,32 +35060,31 @@
       }
     },
     "plugins/node/opentelemetry-instrumentation-mongodb/examples": {
       "name": "mongodb-example",
       "version": "0.28.0",
       "extraneous": true,
       "license": "Apache-2.0",
       "dependencies": {
         "@opentelemetry/api": "^1.0.0",
         "@opentelemetry/exporter-jaeger": "^1.0.0",
         "@opentelemetry/exporter-zipkin": "^1.0.0",
         "@opentelemetry/instrumentation": "^0.48.0",
         "@opentelemetry/instrumentation-http": "^0.48.0",
         "@opentelemetry/instrumentation-mongodb": "^0.32.0",
         "@opentelemetry/sdk-trace-base": "^1.0.0",
-        "@opentelemetry/sdk-trace-node": "^1.0.0",
-        "mongodb": "^3.6.11"
+        "@opentelemetry/sdk-trace-node": "^1.0.0"
       },
       "devDependencies": {
         "cross-env": "^7.0.3",
         "ts-node": "^10.6.0",
         "typescript": "4.4.4"
       },
       "engines": {
         "node": ">=8.12.0"
       }
     },
     "plugins/node/opentelemetry-instrumentation-mongodb/node_modules/bl": {
       "version": "2.2.1",
       "resolved": "https://registry.npmjs.org/bl/-/bl-2.2.1.tgz",
       "integrity": "sha512-6Pesp1w0DEX1N550i/uGV/TqucVL4AM/pgThFSN/Qq9si1/DF9aIHs1BxD8V/QU0HoeHO6cQRTAuYnLPKq1e4g==",
       "dev": true,

Then it works:

% npm install --no-save [email protected] && npm ls mongodb
...

% npm ls mongodb
Run `npm audit` for details.
[email protected] /Users/trentm/tm/opentelemetry-js-contrib2
└─┬ @opentelemetry/[email protected] -> ./plugins/node/opentelemetry-instrumentation-mongodb
  └── [email protected] invalid: "3.6.11" from plugins/node/opentelemetry-instrumentation-mongodb

npm ERR! code ELSPROBLEMS
npm ERR! invalid: [email protected] /Users/trentm/tm/opentelemetry-js-contrib2/node_modules/mongodb

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/trentm/.npm/_logs/2024-02-22T19_29_22_304Z-debug-0.log

Basically I think there are at least a couple bugs in npm here:

npm install --package-lock-only does not remove vestigial workspace dir entries
That npm install --no-save ... should not exit successfully when it does not install the request mongodb version. BTW a contributing reason this only happened with [email protected] is because that exact version was installed at the top-level node_modules/mongodb because version 4.17.1 is a pinned dep of the version of mongoose installed by instrumentation-mongoose.

The only things I know to do here are:

rm and re-create package-lock.json, or
manually edit out those ".../examples" entries from package-lock.json

The latter seems fraught -- there is a possible whole dep tree that I don't
want to try to correctly clean up. So I did the former:

% rm package-lock.json
% npm install --package-lock-only
% git diff package-lock.json | wc -l
   51786

Sigh. The result is an inpenetrable 52k line diff. (IME, npm package-lock updates are unstable on whether some fields are included, e.g.: resolved, integrity, license. The package-lock stability could be because I'm jumping around on npm versions, but I'm not aware of docs here.)

We do see the removal of the vestigial ".../examples/" entries, e.g.:

-    "plugins/node/opentelemetry-instrumentation-mongodb/examples": {
-      "name": "mongodb-example",
-      "version": "0.28.0",
-      "extraneous": true,
-      "license": "Apache-2.0",
-      "dependencies": {
-        "@opentelemetry/api": "^1.0.0",
-        "@opentelemetry/exporter-jaeger": "^1.0.0",
-        "@opentelemetry/exporter-zipkin": "^1.0.0",
-        "@opentelemetry/instrumentation": "^0.48.0",
-        "@opentelemetry/instrumentation-http": "^0.48.0",
-        "@opentelemetry/instrumentation-mongodb": "^0.32.0",
-        "@opentelemetry/sdk-trace-base": "^1.0.0",
-        "@opentelemetry/sdk-trace-node": "^1.0.0",
-        "mongodb": "^3.6.11"
-      },
-      "devDependencies": {
-        "cross-env": "^7.0.3",
-        "ts-node": "^10.6.0",
-        "typescript": "4.4.4"
-      },
-      "engines": {
-        "node": ">=8.12.0"
-      }
-    },

trentm · 2024-02-22T20:10:10Z

.release-please-manifest.json

reviewer note: This change removes the non-publishing package entries that were added in #1939.

trentm · 2024-02-22T20:11:12Z

examples/express/README.md

reviewer note: This diff makes it look like a total re-write, but I only changed the "Installation" section.

trentm · 2024-02-22T20:12:48Z

examples/express/package.json

@@ -8,13 +8,12 @@
    "zipkin:client": "cross-env EXPORTER=zipkin ts-node src/client.ts",
    "jaeger:server": "cross-env EXPORTER=jaeger ts-node src/server.ts",
    "jaeger:client": "cross-env EXPORTER=jaeger ts-node src/client.ts",
-    "compile": "tsc -p .",
-    "setup": "cd ../../../../ && npm ci && cd plugins/node/opentelemetry-instrumentation-express && npm run compile && cd examples && npm run compile"


reviewer note: (/cc @JamieDanielson) this "setup" script is no longer necessary. Now that the examples are not part of the workspace, the user just needs to npm install.

trentm · 2024-02-22T20:19:18Z

package-lock.json

reviewer note: Good luck reviewing this. :) Here is the most succinct version of the diff I could make: https://gist.github.com/trentm/92c639ca7cd86722268d4d4390cecea2

Any hand crafted changes in there or just what npm created?

Just what npm created.

'undici-types', required by the version of @types/node used by the new instrumentation-perf-hooks, was lost in the merge from main.

trentm · 2024-02-24T01:12:04Z

"Unit Tests / unit-test (14) (pull_request) " failure

This is in the new instrumentation-perf-hooks:

> @opentelemetry/[email protected] test
> nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'



  nodejs.event_loop.utilization
    ✓ should stop exporting metrics when disabled
    ✓ should not export immediately after enable
    ✓ can use default eventLoopUtilizationMeasurementInterval
    1) should export event loop utilization metrics after eventLoopUtilizationMeasurementInterval


  3 passing (84ms)
  1 failing

  1) nodejs.event_loop.utilization
       should export event loop utilization metrics after eventLoopUtilizationMeasurementInterval:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/event_loop_utilization.test.ts:94:12)
      at runNextTicks (internal/process/task_queues.js:60:5)
      at processTimers (internal/timers.js:497:9)

So I'm guessing that test is flaky. I have not repro'd locally yet.

trentm · 2024-02-25T20:44:23Z

So I'm guessing that test is flaky. I have not repro'd locally yet.

The test failure was on this line:

assert.strictEqual(metrics[0].dataPoints[0].value < 1, true);

So I wonder if the new nodejs.event_loop.utilization metric can unexpectedly be higher than 1. Bug in the math for that metric?

…ate PR

trentm self-assigned this Feb 14, 2024

trentm mentioned this pull request Feb 14, 2024

updating @opentelemetry/* deps is currently problematic #1917

Closed

trentm added 4 commits February 21, 2024 07:34

chore: remove the non-publishing packages from the npm workspace

7731200

Because they cause difficulties with updating `@opentelemetry/*` dependencies. This is "Option 3" from open-telemetry#1917 Refs: open-telemetry#1917

undo open-telemetry#1917 option 1 work (adding non-publishing package…

5edcef2

…s to release-please config) from open-telemetry#1928

move express example (haven't adjusted build / docs yet)

0cc9047

express example updates to get it working

35ba546

trentm force-pushed the tm-1917-option-3 branch from a690109 to 35ba546 Compare February 21, 2024 16:58

github-actions bot added the pkg:instrumentation-express label Feb 21, 2024

github-actions bot assigned JamieDanielson and pkanal Feb 21, 2024

github-actions bot requested review from JamieDanielson and pkanal February 21, 2024 16:58

trentm added 3 commits February 21, 2024 14:23

remove non-publishing packages from release-please manifest

e44c0b7

move koa example

e8ceb3b

fix up the koa-example-related files

6faae32

github-actions bot added the pkg:instrumentation-koa label Feb 21, 2024

trentm added 2 commits February 21, 2024 14:41

move the mongodb/examples files to the top-level

ce5ca4c

fixup the mongodb-example files

d6f0e77

github-actions bot assigned osherv Feb 21, 2024

github-actions bot added the pkg:instrumentation-mongodb label Feb 21, 2024

github-actions bot requested a review from osherv February 21, 2024 22:51

trentm added 4 commits February 21, 2024 15:36

move mysql-example files

5151d10

fixup the mysql-example files

ff1085a

move redis-example files to top level examples/ dir

ba18ef2

fixup redis-example files

c8aea43

github-actions bot added pkg:instrumentation-mysql pkg:instrumentation-redis labels Feb 22, 2024

github-actions bot requested a review from haddasbronfman February 22, 2024 00:11

trentm added 2 commits February 21, 2024 16:16

update examples README to no longer suggest moving to instruemtnation…

a093303

… package dirs

trentm marked this pull request as ready for review February 22, 2024 20:08

trentm requested a review from a team February 22, 2024 20:08

compile:examples is no longer a thing

05de7e3

trentm commented Feb 22, 2024

View reviewed changes

This was referenced Feb 22, 2024

chore(deps): update dependency test-all-versions to v6.1.0 #1943

Merged

chore: release main #1935

Merged

Flarna approved these changes Feb 23, 2024

View reviewed changes

Merge branch 'main' into tm-1917-option-3

961949a

trentm mentioned this pull request Feb 23, 2024

fix(instr-perf-hooks): avoid 'prepare' npm script because that runs on 'npm install' #1955

Merged

trentm added 2 commits February 23, 2024 12:04

fix for 'npm ci' failures imported from PR open-telemetry#1955

f5271d2

regenerate package-lock.json

8c457bd

'undici-types', required by the version of @types/node used by the new instrumentation-perf-hooks, was lost in the merge from main.

trentm added 2 commits February 25, 2024 12:38

add some more test output information to try to help debug flaky test

569a74d

fix tweaks to the test

c2914f9

trentm mentioned this pull request Feb 26, 2024

flaky instrumentation-runtime-node test #1960

Closed

trentm added 2 commits February 26, 2024 09:09

Merge branch 'main' into tm-1917-option-3

6c530c6

undo the instr-perf-hooks test assert tweaks, leaving that to a separ…

7c54d12

…ate PR

JamieDanielson approved these changes Feb 26, 2024

View reviewed changes

Merge branch 'main' into tm-1917-option-3

6db8f7b

pichlermarc approved these changes Feb 27, 2024

View reviewed changes

pichlermarc merged commit d9af321 into open-telemetry:main Feb 27, 2024
17 checks passed

trentm deleted the tm-1917-option-3 branch February 27, 2024 13:27

trentm mentioned this pull request Mar 12, 2024

fix(mongodb): mongodb v6.4.0 & 6.5.0 support #2001

Merged

JamieDanielson mentioned this pull request Apr 17, 2024

Move package-specific examples under the respective packages #552

Closed

2 tasks

trivikr mentioned this pull request Nov 21, 2024

chore: add workflow to format using prettier #2545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove the non-publishing packages from the npm workspace #1938

chore: remove the non-publishing packages from the npm workspace #1938

trentm commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

pichlermarc commented Feb 19, 2024

trentm commented Feb 21, 2024

trentm commented Feb 22, 2024 •

edited

Loading

trentm Feb 22, 2024

trentm Feb 22, 2024

trentm Feb 22, 2024

trentm Feb 22, 2024 •

edited

Loading

Flarna Feb 23, 2024

trentm Feb 23, 2024

trentm commented Feb 24, 2024

trentm commented Feb 25, 2024

chore: remove the non-publishing packages from the npm workspace #1938

chore: remove the non-publishing packages from the npm workspace #1938

Conversation

trentm commented Feb 14, 2024 • edited Loading

summary of changes

checklist (done)

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

pichlermarc commented Feb 19, 2024

trentm commented Feb 21, 2024

trentm commented Feb 22, 2024 • edited Loading

trentm Feb 22, 2024

Choose a reason for hiding this comment

trentm Feb 22, 2024

Choose a reason for hiding this comment

trentm Feb 22, 2024

Choose a reason for hiding this comment

trentm Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Flarna Feb 23, 2024

Choose a reason for hiding this comment

trentm Feb 23, 2024

Choose a reason for hiding this comment

trentm commented Feb 24, 2024

"Unit Tests / unit-test (14) (pull_request) " failure

trentm commented Feb 25, 2024

trentm commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

trentm commented Feb 22, 2024 •

edited

Loading

trentm Feb 22, 2024 •

edited

Loading