updating @opentelemetry/* deps is currently problematic
#1917
Comments
Not sure if this can work because the minor version part is seen a major from semver point of view for pre 1.0.0 version. Therefore such a range might pull in breaking changes. Regarding non published packages: From my point of view it's better to remove them to streamline the work on the productive code. Maybe we could refer to some bigger, even non JS specific OTel showcase somewhere instead? |
I think the answer is yes, if we ensure that the packages are set to private. Also we're publishing to the
I agree with Flarna here, it's something we should consider.
I need to try something first, but I think I have another option that we could consider. I'll report back here with my findings. |
With this option I'm saying that the intent would be to say "this unpublished examples/ package is meant to work with the current version of Another option is to not have the "examples/package.json". So if a given instrumentation wants to have some examples, then it can, but not as a separate package. If it needs deps for the examples, it can add them to "devDependencies" of the parent instrumentation package.
I don't have a strong opinion either way on the examples. I agree most of them are out of date. The express and fastify examples were updated in the last few weeks. As a total guess: I think most of them will work just fine if their Even if examples are dropped, there is still the separate |
I don't think there are any other options. 😬 Everything I've tried turned out to be a dead end. 😞 IMO we should go with Option 1: It's possible to turn off GitHub releases for certain packages, which we can do via "packages/sampler-aws-xray": { "skip-github-release": true },This way we can skip the GitHub release and tagging in release please. The docs for it say that we'll still need to tag it ourselves when using This way:
We can likely do the same with the examples. Any examples that make our lives difficult beyond this point, we can archive to |
…al deps get bumped This is option 1 from open-telemetry#1917. Refs: open-telemetry#1917
|
#1928 is an attempt at Option 1. |
…opentelemetry/* deps The motivation here was that dependabot doesn't handle this group update for a repo using npm workspaces (see #58 and experiences with opentelemetry-js-contrib, open-telemetry/opentelemetry-js-contrib#1917)
…opentelemetry/* deps (#68) The motivation here was that dependabot doesn't handle this group update for a repo using npm workspaces (see #58 and experiences with opentelemetry-js-contrib, open-telemetry/opentelemetry-js-contrib#1917)
|
I realize this issue is closed but mentioning here as promised on the release PR. We might have to go with Option 3 after all, though I've missed the detail around the aws sampler package and suspect that makes it a trickier option. |
|
Re-opening. I thikn this is still open for discussion. |
|
Maybe we should move the aws xray sampler into archive folder? Or into a in-development folder? |
Yah, I was working on a quick PR for discussion doing Option 3. Part of it moved sampler-aws-xray to an "incubator/" dir. I don't have a strong opinion on what directory name to use. I don't know if the existing "archive/" dir is appropriate for sampler-aws-xray? |
|
I think |
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
|
#1938 is a draft PR (still incomplete) doing Option 3. Thoughts? |
…in release-please release PRs This continues on from open-telemetry#1928. The non-releasing packages are being added to the release-please manifest in the hope that this prevents them from being listed in the set of released packages in a release-please PR. This is effectively Option 1b from open-telemetry#1917.
…in release-please release PRs (#1939) This continues on from #1928. The non-releasing packages are being added to the release-please manifest in the hope that this prevents them from being listed in the set of released packages in a release-please PR. This is effectively Option 1b from #1917. Co-authored-by: Marc Pichler <[email protected]>
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
* chore: remove the non-publishing packages from the npm workspace Because they cause difficulties with updating `@opentelemetry/*` dependencies. This is "Option 3" from #1917 Refs: #1917 * undo #1917 option 1 work (adding non-publishing packages to release-please config) from #1928 * move express example (haven't adjusted build / docs yet) * express example updates to get it working * remove non-publishing packages from release-please manifest * move koa example * fix up the koa-example-related files * move the mongodb/examples files to the top-level * fixup the mongodb-example files * move mysql-example files * fixup the mysql-example files * move redis-example files to top level examples/ dir * fixup redis-example files * update examples README to no longer suggest moving to instruemtnation package dirs * regenerate package-lock.json to rm the '.../examples' dir entries '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]. * compile:examples is no longer a thing * fix for 'npm ci' failures imported from PR #1955 * regenerate package-lock.json 'undici-types', required by the version of @types/node used by the new instrumentation-perf-hooks, was lost in the merge from main. * add some more test output information to try to help debug flaky test * fix tweaks to the test * undo the instr-perf-hooks test assert tweaks, leaving that to a separate PR
* chore: add script to update '@opentelemetry/*' deps in this repo Refs: #1917 * add explicit top-level devDeps for modules used by update-otel-deps.js * updates from Marc's feedback
|
@pichlermarc I think this is good now. Please re-open if there are still problems updating |
Updating
@opentelemetry/*deps in the contrib repo is currently problematic.There are two kinds of updates here:
@opentelemetry/*deps that come from the core-repo (opentelemetry-js.git). This is typically done in a PR created after a release of packages from the core repo. In the past this was done via renovatebot, but since the switch to npm workspaces this has been problematic.@opentelemetry/*deps that live in this same contrib-repo (internal deps). For example some packages depend on@opentelemetry/contrib-test-utilsfor testing;@opentelemetry/auto-instrumentations-nodedepends on all theinstrumentation-*deps that live in this repo. This update is typically done by a release PR (handled via the release-please tool).problems
The initial motivating problem was that attempts by renovatebot to do an update of OTel deps from the core repo was failing to update everything. For example, an attempt to update deps to versions 0.47.0/1.20.0 would leave parts of the contrib repo with deps at 1.19.0. Mixed 1.19.0 and 1.20.0 deps would cause breakages, e.g. type mismatches. See #1905 and then the follow-up #1906.
However, there are other issues, that I think stem from related causes.
Here are two examples of current silly/problematic behaviour.
First, updating the
@opentelemetry/instrumentation-bunyandep in its "examples/" package crashes npm.In a clone of the contrib repo at the current "main" (commit 497a3c3):
(That npm log file is here: https://gist.github.com/trentm/c75060cfe647c6a3070fca1b1b589eec
It crashes with npm v10 -- included in Node v18.19.0 -- as well.)
Second, updating the
@opentelemetry/contrib-test-utilsdep inpackages/opentelemetry-sampler-aws-xrayrequires running npm install twice:There are changes to the package-lock.json in each of those
npm install ...steps that I haven't shown.Both of these problems make scripting the update of OTel deps difficult.
causes
The two silly cases above ultimately are probably npm bugs.
However, there are two complicating factors in the contrib repo:
There are some workspace packages that aren't updating internal dependencies.
@opentelemetry/contrib-test-utils, but because the package isn't currently being released, it isn't part of the release-please PR that updates internal deps.@opentelemetry/instrumentation-express). These examples packages are not published, so they are currently not part of the release-please PR that updates internal deps.The result is that contrib-repo package versions move on (e.g.
@opentelemetry/[email protected]is released), but the dep ranges in these non-publishing packages get stuck at an older version (e.g. bunyan example's dep is stuck at "^0.34.0"). The transitive deps of@opentelemetry/[email protected]are to older core-repo versions, but the direct core-repo deps in bunyan examples are updated to the latest: resulting in conflicting versions of deps.The "plugins/node/*/examples/package.json" packages are part of the npm workspace, but they have another workspace package in a parent dir. For example both "plugins/node/opentelemetry-instrumentation-bunyan" and its subdir "plugins/node/opentelemetry-instrumentation-bunyan/examples" are workspace packages. My guess is that npm just isn't well-tested for this case. When determining the
node_modules/tree layout, it needs to handle the case of the examples/ dir "seeing" the parent dir'snode_modules/.... The result is some surprising, to me, layouts and the npm crash shown above.options
1. Update the internal
@opentelemetry/*even for the non-publishing packages.This would mean that a release PR that bumps the express instrumentation should also update the examples/package.json, e.g.:
This might just require adding the "sampler-aws-xray" and "*/examples/" dirs to "release-please-config.json", but I'm not sure if release-please will then attempt to
npm publishthese private packages or have other issues.2. Less tightly specify contrib-repo deps in the non-publishing packages.
E.g.:
This would result in the "sampler-aws-xray" and "examples/" packages always using the latest internal deps.
3. Remove the non-publishing packages from the npm workspace
E.g. this patch would do it for the "examples/" dirs, but not for the "sampler-aws-xray" package:
This would take them out of the package-lock.json file and decouple them from releases and updates. It would take separate work to keep those package deps up to date (which could separately use dependabot/renovate).
For better or worse, this would mean that examples/ packages would not use the instrumentation code in the parent dir.
questions
Q1: @pichlermarc, do you know if adding these non-publishing packages to the release-please config would work as we want: update the internal deps, but still not published them?
Q2: Does anyone have opinions on the options above, or options I've missed?
The text was updated successfully, but these errors were encountered: