chore: track package-lock.json #4238

legendecas · 2023-10-30T09:29:36Z

Which problem is this PR solving?

Check-in package-lock.json to avoid breakage on regular CI runs. Dependencies should be updated regularly with renovate.

Fixes #785

Notable Changes:

lerna bootstrap is replaced by npm install with npm workspaces.
@lerna/legacy-package-management is removed.
lerna is degraded to v6.6.2 to be consistent across all supported Node.js versions.
@grpc/grpc-js is locked to v1.8.21 in package-lock.json.
The compatibility test using @opentelemetry/resource v1.9.0 (requires a peer @api < 1.5.0) in sdk-trace-base is temporarily disabled.

Type of change

Internal

Checklist:

Followed the style guidelines of this project

codecov · 2023-10-30T09:33:08Z

Codecov Report

Merging #4238 (1864f1d) into main (1c7d7a3) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 1864f1d differs from pull request most recent head a639048. Consider uploading reports for the commit a639048 to get more accurate results

@@            Coverage Diff             @@
##             main    #4238      +/-   ##
==========================================
- Coverage   92.24%   92.18%   -0.07%     
==========================================
  Files         331      319      -12     
  Lines        9481     8829     -652     
  Branches     1999     1875     -124     
==========================================
- Hits         8746     8139     -607     
+ Misses        735      690      -45

see 14 files with indirect coverage changes

dyladan · 2023-10-30T15:56:27Z

I think I know what is failing the node 14 tests. If it's ok with you I'll just update your branch rather than creating a new PR since this is high priority and blocking all PRs

dyladan · 2023-10-30T19:12:44Z

Looks like the test is going to time out. It must be stuck somehow. running locally with the same changes results in a failure of the tree shaking test on my machine. I would appreciate if someone else could try to verify the behavior is the same on a windows or linux machine.

Flarna · 2023-10-31T08:15:12Z

tried on windows using the steps as in CI but it fails early during npm ci --ignore-scripts with

npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.

Not sure but maybe the previous npm install --save-dev nx@^15 lerna@^6 causes some issues with the lock files.

I think we should aim to get the build setup back into a state where a simple npm install should install what is needed. If we need old version we should use it for all node versions. if we need special learn hoist configs we should use it local and in ci.
The current state to start a local build with reverse engineering github actions files is a bit cumbersome.

dyladan · 2023-10-31T12:07:32Z

Not sure but maybe the previous npm install --save-dev nx@^15 lerna@^6 causes some issues with the lock files.

This should update the lockfiles so it shouldn't have that problem. I don't get that when I run it on mine and it doesn't happen on the CI either.

I think we should aim to get the build setup back into a state where a simple npm install should install what is needed. If we need old version we should use it for all node versions. if we need special learn hoist configs we should use it local and in ci.
The current state to start a local build with reverse engineering github actions files is a bit cumbersome.

We only need to do the special steps for node 14. We were currently talking in the maintainer channel about the idea to simply stop testing 14 because of this problem. In the past when we've deprecated a node runtime without a major version we've gotten strong pushback, so we're trying to figure out other options first.

So far we have:

bend the CI around to make 14 work
stop testing 14. possibly issue some deprecation "we aren't testing this" warning as an npm postinstall script on node 14
revert testing dependencies far enough back that they work on node 14. this likely involves switching from lerna to nx in order to have stricter control over which version of nx we use (lerna just uses nx under the hood) or going to a very old lerna version (<=6).

.github/workflows/unit-test.yml

Flarna · 2023-10-31T14:28:13Z

retried with a really clean setup now.
Locally node 14 unittests are green for me now on windows.

Flarna · 2023-10-31T15:33:54Z

maybe worth to mention that I used the npm version coming with node. In my case it's npm 6.14.18 and node 14.21.3

dyladan · 2023-10-31T17:35:00Z

maybe worth to mention that I used the npm version coming with node. In my case it's npm 6.14.18 and node 14.21.3

I'm using the same and getting failures on a complete fresh build in macos following the exact steps in the workflow

         1114 passing (3s)
         1 failing
       
         1) tree-shaking
              verify MetricsAPI:
            Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/daniel.dyla/otel/opentelemetry-js/api/test/tree-shaking/tree-shaking.test.ts)

The test failing is one that deals with mocking filesystems so it's entirely possible this is an OS level issue for me. The CI logs don't make it easy to see which test is causing the problem but it seems like it hangs forever. There are no tests that time out though so it appears to be hanging between tests or something? Maybe all tests are completed and the runtime just never finishes.

dyladan · 2023-10-31T17:37:18Z

I'm trying to remove the cache. Because we're using lockfiles, the node_modules is removed when bootstrap is run anyway and it is one variable we can remove for now.

dyladan · 2023-11-01T13:43:17Z

I pushed a change to break the cache to see if i'm just doing something wrong locally

dyladan · 2023-11-01T14:06:26Z

My cache break didn't break the build so I must be doing something locally causing the problem.

edit: went through the steps again and it worked this time. I think the important step that I missed was not updating the global NPM installation

legendecas · 2023-11-01T14:15:33Z

I installed deps with npm i (note, without any flags, like --ignore-scripts) and it works well. However, if flags like --ignore-scripts are set, it doesn't work like you mentioned because now dependencies of all sub-packages are installed together with the --ignore-scripts flag and it would break a lot.

Would you mind verifying the install command you use?

dyladan · 2023-11-01T14:21:20Z

I installed deps with npm i (note, without any flags, like --ignore-scripts) and it works well. However, if flags like --ignore-scripts are set, it doesn't work like you mentioned because now dependencies of all sub-packages are installed together with the --ignore-scripts flag and it would break a lot.

Would you mind verifying the install command you use?

Going through a clean install following the exact steps I was able to build and test fine. There is a test failure but I believe it is macos specific (it is the tree shaking test) so I would prefer to unblock the CI and resolve it in a separate PR.

Here are my exact commands I used for reference which worked:

# make sure directory is completely clean
rm -rf node_modules packages/*/node_modules experimental/packages/*/node_modules && git clean -ffdx

# install npm 10
npm install -g npm@"<10.0.0"

# install dependencies
npm ci

# compile packages
npm run compile

# test
npm run test

dyladan · 2023-11-01T14:41:54Z

Successful in 3m

Seems fast enough anyway without caches

dyladan

Thanks for doing this. CI is always a headache.

.github/workflows/benchmark.yml

dyladan · 2023-11-01T15:06:17Z

OK to merge this whenever you're comfortable IMO. It's blocking other things so its high priority.

t2t2 · 2023-11-01T16:26:40Z

package.json

    "submodule": "git submodule sync --recursive && git submodule update --init --recursive",
-    "version:update": "lerna run version:update",
+    "version:update": "lerna run version",


npm native version:

Suggested change

"version:update": "lerna run version",

"version:update": "npm run --ws --if-present version",

(I assume lack of --if-present was the error mentioned in sig, worked with this in gh workspace of this PR)

optionally can add -- at end if you want to pass flags forward (npm run version:update --something -> npm run version --something in workspaces) but it's not necessary here

Suggested change

"version:update": "lerna run version",

"version:update": "npm run --ws --if-present version --",

Yes, it works in a more verbose manner. And worth mentioning that npm run doesn't parallel the execution so it is much slower than lerna run -- this is the main reason I didn't touch lerna run.

Good point, tho at speed of version update script parallel's kinda whatever

yeah... the speed problem is about test, not version.

trentm · 2023-11-01T18:09:04Z

Aside: Is there a separate issue for the test hang with @grpc/grpc-js@1.9.x?

trentm

LGTM. Tested with node 18 & 16 on macos.

trentm · 2023-11-01T18:53:59Z

.github/workflows/peer-api.yml

      - name: Install lerna
-        run: npm install -g lerna
+        run: npm install -g lerna@6.6.2

      - name: Install semver
        run: npm install semver


nit: This npm install semver installs all dependencies anyway (npm issue: npm/cli#3023). This could consider just running npm ci OR also installing semver globally (untested): npm install -g semver. The latter, if it works, would avoid installing ~800MB of deps.

trentm · 2023-11-01T23:31:40Z

This also closes #3927 right?

- use npm workspaces instead of lerna for monorepo install handling (requires at least npm@7, so node v14 users will have to update their npm) - updates to lerna@6, which brings parallel - add package-lock.json for more reliable dev and testing Refs: open-telemetry/opentelemetry-js#4238

* chore: track package-lock.json (#4238) * chore: track package-lock.json * Pin to old versions for node 14 * Use version range * Remove unused cached directories * Temporarily disable other tests * Temporarily enable only api test * Enable only some packages * Test only api packages * Test trace exporters * Fix line ordering * Test all packages except otlp exporters * Add trace http exporter * Add trace proto exporter * Test all but grpc exporters * chore: use npm workspaces and degrade lerna to v6 * chore: get rid of lerna bootstrap * chore: use npx * chore: allow install scripts to setup buf * chore: fix w3c-integration-test cache key * chore: fix cache key * chore: disable resource compat test * chore: fix node_modules assumptions * chore: fix hoisted karma issue * chore: fix markdown linter complaints * chore: lock @grpc/grpc-js to v1.8.21 * Break caches * chore: remove cache * chore: fixup inline commands --------- Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * docs: fixed link to benchmark results (#4233) Co-authored-by: Chengzhong Wu <legendecas@gmail.com> * chore(deps): update all patch versions (#4215) * fix: otlp json encoding (#4220) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * fix: remove duplicate export star from version.ts (#4225) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * docs: fix sdk-node config instructions (#4249) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * feat(api): publish api esnext target (#4231) * chore: release API 1.7.0/Core 1.18.0/Experimental 0.45.0 (#4254) * fix(sdk-metrics): hand-roll MetricAdvice type as older API versions do not include it (#4260) * chore: prepare release 1.18.1/0.45.1 (#4261) * chore: no need for 'packages' in "lerna.json" (#4264) * Benchmark tests for trace OTLP transform and BatchSpanProcessor (#4218) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * chore: type reference on zone.js (#4257) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * docs: add docker-compose to run prometheus for the experimental example (#4268) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * fix(sdk-logs): avoid map attribute set when count limit exceeded (#4195) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> * chore(deps): update dependency chromedriver to v119 [security] (#4280) * chore(deps): update actions/setup-node action to v4 (#4236) * fix(sdk-trace-base): processor onStart called with a span having empty attributes (#4277) Co-authored-by: artahmetaj <artahmetaj@yahoo.com> * Update fetch instrumentation to be runtime agnostic (#4063) Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> --------- Co-authored-by: Chengzhong Wu <legendecas@gmail.com> Co-authored-by: Martin Kuba <martin@martinkuba.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Siim Kallas <siimkallas@gmail.com> Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com> Co-authored-by: David Luna <david.luna@elastic.co> Co-authored-by: Dinko Osrecki <dinko.osrecki@emarsys.com> Co-authored-by: Trent Mick <trentm@gmail.com> Co-authored-by: François <32224751+Lp-Francois@users.noreply.github.com> Co-authored-by: Hyun Oh <hyunnoh01@gmail.com> Co-authored-by: André Cruz <andremiguelcruz@msn.com> Co-authored-by: artahmetaj <artahmetaj@yahoo.com> Co-authored-by: drewcorlin1 <82601620+drewcorlin1@users.noreply.github.com>

legendecas requested a review from a team October 30, 2023 09:29

legendecas marked this pull request as draft October 30, 2023 09:31

legendecas force-pushed the package-lock branch 3 times, most recently from 22c7915 to e7c9c01 Compare October 30, 2023 09:52

chore: track package-lock.json

1c9205f

legendecas force-pushed the package-lock branch from e7c9c01 to 1c9205f Compare October 30, 2023 09:55

dyladan mentioned this pull request Oct 30, 2023

Switch from lerna to nx #4240

Open

Pin to old versions for node 14

de24a33

dyladan reviewed Oct 31, 2023

View reviewed changes

.github/workflows/unit-test.yml Outdated Show resolved Hide resolved

Use version range

b96c3d9

Remove unused cached directories

1dbf56d

dyladan added 9 commits October 31, 2023 14:00

Temporarily disable other tests

6f7706a

Temporarily enable only api test

2edd213

Enable only some packages

68931bf

Test only api packages

0bb77a0

Test trace exporters

7aaa168

Fix line ordering

375ac6c

Test all packages except otlp exporters

1041126

Add trace http exporter

da4ac69

Add trace proto exporter

6b18e61

chore: remove cache

ba0d37b

dyladan approved these changes Nov 1, 2023

View reviewed changes

dyladan reviewed Nov 1, 2023

View reviewed changes

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved

chore: fixup inline commands

a639048

MSNev approved these changes Nov 1, 2023

View reviewed changes

JamieDanielson approved these changes Nov 1, 2023

View reviewed changes

t2t2 reviewed Nov 1, 2023

View reviewed changes

trentm approved these changes Nov 1, 2023

View reviewed changes

dyladan linked an issue Nov 1, 2023 that may be closed by this pull request

Migrate to npm workspaces #3927

Closed

dyladan mentioned this pull request Nov 1, 2023

[CI] Avoid installing unnecessary dependencies #4242

Closed

dyladan merged commit e9328ab into open-telemetry:main Nov 1, 2023
17 checks passed

dyladan mentioned this pull request Nov 1, 2023

[CI] enable npm-cache #4243

Closed

legendecas deleted the package-lock branch November 2, 2023 02:23

legendecas mentioned this pull request Nov 2, 2023

[In Progress] Internal: lerna migration #4067

Closed

8 tasks

trentm mentioned this pull request Nov 2, 2023

fix: otlp json encoding #4220

Merged

3 tasks

trentm mentioned this pull request Nov 3, 2023

chore: use npm workspaces and a package-lock.json open-telemetry/opentelemetry-js-contrib#1771

Merged

pichlermarc mentioned this pull request Nov 6, 2023

chore: release API 1.7.0/Core 1.18.0/Experimental 0.45.0 #4254

Merged

This was referenced Nov 6, 2023

chore(deps): update dependency lerna to v7 #4251

Closed

chore(deps): update actions/setup-node action to v4 #4236

Merged

pichlermarc mentioned this pull request Nov 9, 2023

chore(renovate): require dashboard approval for lerna updates #4276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: track package-lock.json #4238

chore: track package-lock.json #4238

legendecas commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 30, 2023 •

edited

Loading

dyladan commented Oct 30, 2023

dyladan commented Oct 30, 2023

Flarna commented Oct 31, 2023

dyladan commented Oct 31, 2023

Flarna commented Oct 31, 2023

Flarna commented Oct 31, 2023

dyladan commented Oct 31, 2023

dyladan commented Oct 31, 2023

dyladan commented Nov 1, 2023

dyladan commented Nov 1, 2023 •

edited

Loading

legendecas commented Nov 1, 2023

dyladan commented Nov 1, 2023

dyladan commented Nov 1, 2023

dyladan left a comment

dyladan commented Nov 1, 2023

t2t2 Nov 1, 2023

legendecas Nov 1, 2023

t2t2 Nov 1, 2023

legendecas Nov 1, 2023

trentm commented Nov 1, 2023

trentm left a comment

trentm Nov 1, 2023

dyladan Nov 1, 2023

trentm commented Nov 1, 2023

	"version:update": "lerna run version",
	"version:update": "npm run --ws --if-present version",

chore: track package-lock.json #4238

chore: track package-lock.json #4238

Conversation

legendecas commented Oct 30, 2023 • edited Loading

Which problem is this PR solving?

Type of change

Checklist:

codecov bot commented Oct 30, 2023 • edited Loading

Codecov Report

dyladan commented Oct 30, 2023

dyladan commented Oct 30, 2023

Flarna commented Oct 31, 2023

dyladan commented Oct 31, 2023

Flarna commented Oct 31, 2023

Flarna commented Oct 31, 2023

dyladan commented Oct 31, 2023

dyladan commented Oct 31, 2023

dyladan commented Nov 1, 2023

dyladan commented Nov 1, 2023 • edited Loading

legendecas commented Nov 1, 2023

dyladan commented Nov 1, 2023

dyladan commented Nov 1, 2023

dyladan left a comment

Choose a reason for hiding this comment

dyladan commented Nov 1, 2023

t2t2 Nov 1, 2023

Choose a reason for hiding this comment

legendecas Nov 1, 2023

Choose a reason for hiding this comment

t2t2 Nov 1, 2023

Choose a reason for hiding this comment

legendecas Nov 1, 2023

Choose a reason for hiding this comment

trentm commented Nov 1, 2023

trentm left a comment

Choose a reason for hiding this comment

trentm Nov 1, 2023

Choose a reason for hiding this comment

dyladan Nov 1, 2023

Choose a reason for hiding this comment

trentm commented Nov 1, 2023

legendecas commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 30, 2023 •

edited

Loading

dyladan commented Nov 1, 2023 •

edited

Loading