compile and test fail with lerna deprecated attribute

[maryliag]

What happened?

Steps to Reproduce

Cloned the repo, install dependencies and try compile npm run compile or test npm run test, both will fail with message

lerna ERR! ECONFIGWORKSPACES The "useWorkspaces" option has been removed. By default lerna will 
resolve your packages using your package manager's workspaces configuration. Alternatively, you can 
manually provide a list of package globs to be used instead via the "packages" option in lerna.json.

Since useWorkspaces was removed here, this attribute needs to be removed from lerna.json

Expected Result

Being able to compile

[trentm]

@maryliag Thanks for the issue.

IIUC that useWorkspaces removal was only in lerna v7. This repo is pinning lerna to v6.6.2 (https://github.com/open-telemetry/opentelemetry-js/blob/3a426e8c32b9a3db691ba4225dadc52de62e660f/package.json#L85).

I would think any of the npm run ... commands would result in using the locally installed lerna@6.6.2. Are you able to show more details of the exact commands you were running where you hit this. Then we can see if we can defend against it.

FWIW, here is a full run of me doing: clone, npm ci, npm run compile https://gist.github.com/trentm/0f393fa869cf7e4c8c195eafea3b3717

[maryliag]

Hi @trentm , thank you for getting back to me! I used almost the same commands you did, the difference is that once I notice the message with all the vulnerabilities after the npm install I decided to run an audit fix, and I can see that after that it changed the lerna version:

❯ npx lerna --version
8.1.2

So I deleted node modules and reinstalled again without trying to fix all vulnerabilities and it on 6.6.2 as expected. With that I'm able to compile, so I guess I should close this issue (maybe worth actually making the proper updates to fix all these vulnerabilities).

Although without fixing those versions, the npm run test have one test constantly failing

1) Node.js: browserDetector()
should return empty resources if window.document is missing:

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

       3 !== 0
      + expected - actual

      -3
      +0

Are all the tests passing for you?

[trentm]

Are all the tests passing for you?

That particular test (in "packages/opentelemetry-resources") is passing for me. I ran npm test in that dir using node v18.18.2, FWIW.

In CI, all the tests are currently passing for all node versions.

Hrm. If you do the following, does it still fail for you?

cd .../opentelemetry-js  # the base of your clone
npm ci
npm run compile
cd packages/opentelemetry-resources
npm test

[maryliag]

Following your commands I get the same test failure, with same error message.

I'm on node 21.6.2.

opentelemetry-js/packages/opentelemetry-resources on  main [!] is 📦 v1.22.0 via  v21.6.2
❯ npm test

> @opentelemetry/resources@1.22.0 test
> nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'

  detectResourcesSync
    ✔ handles resource detectors which return Promise<Resource>
    ✔ handles resource detectors which return Resource with a promise inside
    Node.js: logging
      ✔ logs when a detector's async attributes promise rejects

  Browser: browserDetector()
    - should return browser information
    - should return empty resources if version is missing

  Browser: envDetector() on web browser
    with valid env
      - should return resource information from environment variable
    with invalid env
      value: 'webengine.description="with spaces"'
        - should return empty resource
    with empty env
      - should return empty resource
    with empty env
      - should return empty resource

  Browser: hostDetector() on web browser
    - should return empty resource

  Browser: osDetector() on web browser
    - should return empty resource

  Browser: processDetector() on web browser
    - should return empty resource

  Node.js: browserDetector()
    1) should return empty resources if window.document is missing

  Node.js: envDetector() on Node.js
    with valid env
      ✔ should return resource information from environment variable
    with invalid env
      value: 'k8s.deployment.name="with spaces"'
        ✔ should return empty resource
    with empty env
      ✔ should return empty resource

  Node.js: hostDetector() on Node.js
    ✔ should return resource information about the host
    ✔ should pass through arch string if unknown
    ✔ should handle missing machine id

  getMachineId on BSD
    ✔ reads machine-id from primary
    ✔ reads machine-id from fallback when primary fails
    ✔ handles failure to read primary and fallback

  getMachineId on Darwin
    ✔ reads machine-id
    ✔ handles failure

  getMachineId on linux
    ✔ reads machine-id from primary
    ✔ reads machine-id from fallback when primary fails
    ✔ handles failure to read primary and fallback

  getMachineId on Windows
    ✔ reads machine-id
    ✔ handles failure

  Node.js: osDetector() on Node.js
    ✔ should return resource information from process
    ✔ should pass through type string if unknown

  Node.js: processDetector() on Node.js
    ✔ should return resource information from process
    ✔ should return a resources if title, command and commandLine are missing

  Regression Test @opentelemetry/resources@1.9.1
    ✔ constructor

  assertCloudResource
    ✔ requires one cloud label
    ✔ validates optional attributes

  assertContainerResource
    ✔ requires one container label
    ✔ validates optional attributes

  assertHostResource
    ✔ requires one host label
    ✔ validates optional attributes

  assertK8sResource
    ✔ requires one k8s label
    ✔ validates optional attributes

  assertTelemetrySDKResource
    ✔ uses default validations
    ✔ validates optional attributes

  assertServiceResource
    ✔ validates required attributes
    ✔ validates optional attributes

  Resource
    ✔ should return merged resource
    ✔ should return merged resource when collision in attributes
    ✔ should return merged resource when first resource is empty
    ✔ should return merged resource when other resource is empty
    ✔ should return merged resource when other resource is null
    ✔ should accept string, number, and boolean values
    ✔ should log when accessing attributes before async attributes promise has settled
    .empty()
      ✔ should return an empty resource (except required service name)
      ✔ should return the same empty resource
      ✔ should return false for asyncAttributesPending immediately
    asynchronous attributes
      ✔ should return false for asyncAttributesPending if no promise provided
      ✔ should return false for asyncAttributesPending once promise settles
      ✔ should merge async attributes into sync attributes once resolved
      ✔ should merge async attributes when both resources have promises
      ✔ should merge async attributes correctly when resource1 fulfils after resource2
      ✔ should merge async attributes correctly when resource2 fulfils after resource1
      ✔ should log when promise rejects
    Node.js: .default()
      ✔ should return a default resource
    Browser: .default()
      - should return a default resource
    compatibility
      ✔ should merge resource with old implementation
      ✔ should merge resource containing async attributes with old implementation

  56 passing (40ms)
  10 pending
  1 failing

  1) Node.js: browserDetector()
       should return empty resources if window.document is missing:

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

3 !== 0

      + expected - actual

      -3
      +0

      at assertEmptyResource (test/util/resource-assertions.ts:375:10)
      at Context.<anonymous> (test/detectors/node/BrowserDetector.test.ts:24:24)

------------------------------------------------|---------|----------|---------|---------|-------------------
File                                            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------------------------------|---------|----------|---------|---------|-------------------
All files                                       |   84.85 |    72.54 |   91.66 |   84.78 |
 opentelemetry-resources                        |       0 |      100 |       0 |       0 |
  karma.worker.js                               |       0 |      100 |       0 |       0 | 17-21
 opentelemetry-resources/src                    |   83.33 |    79.59 |   85.71 |   82.43 |
  Resource.ts                                   |     100 |    93.33 |     100 |     100 | 75,97
  detect-resources.ts                           |   70.45 |       50 |   72.72 |   69.04 | 35-52,87,92-93
  utils.ts                                      |     100 |      100 |     100 |     100 |
 opentelemetry-resources/src/detectors          |      90 |    77.77 |     100 |   91.17 |
  BrowserDetector.ts                            |     100 |      100 |     100 |     100 |
  BrowserDetectorSync.ts                        |   76.92 |       50 |     100 |   76.92 | 29,52-55
  EnvDetector.ts                                |     100 |      100 |     100 |     100 |
  EnvDetectorSync.ts                            |   91.48 |     82.6 |     100 |   93.33 | 73,109,116
  index.ts                                      |     100 |      100 |     100 |     100 |
 opentelemetry-resources/src/platform/browser   |       0 |      100 |     100 |       0 |
  HostDetector.ts                               |       0 |      100 |     100 |       0 | 19
  HostDetectorSync.ts                           |       0 |      100 |     100 |       0 | 19
  OSDetector.ts                                 |       0 |      100 |     100 |       0 | 19
  OSDetectorSync.ts                             |       0 |      100 |     100 |       0 | 19
  ProcessDetector.ts                            |       0 |      100 |     100 |       0 | 19
  ProcessDetectorSync.ts                        |       0 |      100 |     100 |       0 | 19
 opentelemetry-resources/src/platform/node      |   93.22 |    72.72 |     100 |   92.98 |
  HostDetector.ts                               |     100 |      100 |     100 |     100 |
  HostDetectorSync.ts                           |     100 |      100 |     100 |     100 |
  OSDetector.ts                                 |     100 |      100 |     100 |     100 |
  OSDetectorSync.ts                             |     100 |      100 |     100 |     100 |
  ProcessDetector.ts                            |     100 |      100 |     100 |     100 |
  ProcessDetectorSync.ts                        |    92.3 |      100 |     100 |    92.3 | 54
  default-service-name.ts                       |     100 |      100 |     100 |     100 |
  utils.ts                                      |   76.92 |    57.14 |     100 |   72.72 | 21-23,36
 ...etry-resources/src/platform/node/machine-id |   86.56 |       40 |     100 |   86.56 |
  execAsync.ts                                  |     100 |      100 |     100 |     100 |
  getMachineId-bsd.ts                           |     100 |      100 |     100 |     100 |
  getMachineId-darwin.ts                        |   92.85 |       50 |     100 |   92.85 | 29
  getMachineId-linux.ts                         |     100 |      100 |     100 |     100 |
  getMachineId-win.ts                           |   93.33 |       50 |     100 |   93.33 | 26
  getMachineId.ts                               |   41.66 |       20 |     100 |   41.66 | 25-34
------------------------------------------------|---------|----------|---------|---------|-------------------
npm ERR! Lifecycle script `test` failed with error:
npm ERR! Error: command failed
npm ERR!   in workspace: @opentelemetry/resources@1.22.0
npm ERR!   at location: /Users/maryliag/development/otel/opentelemetry-js/packages/opentelemetry-resources

[trentm]

I'm on node 21.6.2.

I can repro that test failure with 21.6.2. We should probably open a separate issue for this.

on the lerna version

Currently we are pinned at lerna 6. AFAIK we cannot upgrade to lerna 8 because it has dropped support for Node v14 and node v16, which this repo currently needs to support. (The coming OTel SDK v2 work might mean we will drop v14 and v16 support.)

But what about lerna 7? (I'm not sure if upgrading to the latest lerna 7 resolves relevant npm audit issues.)

There had been an earlier attempt here: https://github.com/open-telemetry/opentelemetry-js/issues/3894 That was blocked by lerna bootstrap having been removed in lerna 7. However, we don't need that anymore since moving to using npm workspaces, so I suppose it is possible lerna v7 would work now.

[trentm]

I can repro that test failure with 21.6.2.

And currently this repo isn't testing with Node.js versions greater than v18.18.2 because of https://github.com/open-telemetry/opentelemetry-js/issues/4553

[trentm]

That test fails on Node.js v21 because the OTel code is using const isBrowser = typeof navigator !== 'undefined'; and Node.js 21.0.0 added an experimental navigator global: https://nodejs.org/api/all.html#all_globals_navigator

I'll open an issue.

[maryliag]

In that case, I will open an issue for the test failure, indicating the version, so once the node is updated this can be tracked.

For the lerna version, do you want me to close this issue or keep it open (adding clarification on the version), so when lerna gets updated this can be addressed? Because I did open a PR with the fix, so I can keep that open, close, merge, let me know how to proceed: https://github.com/open-telemetry/opentelemetry-js/pull/4557

[trentm]

Oh, I was part way through opening an issue for the test failure already. Either works though.

[trentm]

https://github.com/open-telemetry/opentelemetry-js/issues/4561 added for the test failure.

[trentm]

For the lerna version, do you want me to close this issue or keep it open

I am inclined to closed both this issue and the PR.

I am giving a quick try to see if we can update to lerna v7. However, I'm not sure if that even really helps the issue (npm audit fix wanting to update further) the started this.

[trentm]

I am giving a quick try to see if we can update to lerna v7.

I am giving up on this attempt for now. :/

Some notes:

I updated to lerna@7 roughly with the following command:

npm exec --workspaces --include-workspace-root -- npm install --save-dev lerna@7

I say "roughly" because that also installs a new lerna dep in some of the workspace packages that didn't currently depend on lerna. I manually backed those out via npm uninstall lerna in each of the relevant dirs.

Now I attempt to run npm run compile and npm run lint:

% npm run compile

> opentelemetry@0.1.0 precompile
> lerna run version && npm run submodule

(node:23558) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
lerna notice cli v7.4.2
lerna ERR! TypeError: (0 , import_devkit4.createProjectFileMapUsingProjectGraph) is not a function
lerna ERR!     at detectProjects (/Users/trentm/tm/opentelemetry-js3/node_modules/lerna/dist/index.js:1456:89)
lerna ERR! lerna (0 , import_devkit4.createProjectFileMapUsingProjectGraph) is not a function

First lint attempt:

% npx run lint
Need to install the following packages:
run@1.5.0
Ok to proceed? (y) npm ERR! canceled

npm ERR! A complete log of this run can be found in: /Users/trentm/.npm/_logs/2024-03-20T18_28_09_190Z-debug-0.log

Second attempt:

% npm run lint

> opentelemetry@0.1.0 lint
> lerna run lint

lerna notice cli v7.4.2
lerna ERR! TypeError: (0 , import_devkit4.createProjectFileMapUsingProjectGraph) is not a function
lerna ERR!     at detectProjects (/Users/trentm/tm/opentelemetry-js3/node_modules/lerna/dist/index.js:1456:89)
lerna ERR!     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
lerna ERR! lerna (0 , import_devkit4.createProjectFileMapUsingProjectGraph) is not a function

My uneducated guess is that this is about some modularization in lerna v7, which is now based on nx?

[maryliag]

Thank you for all the details and for trying it out! This is looking more complex that the intended issue I created for, so I will close both the issue and the PR and once lerna gets eventually updated this will probably need to be tested anyway.

Also thank you for opening the issue for the test failure! 😄

[trentm]

@maryliag Thanks for the issue and PR! Yah, unfortunately the dependency story in this repo, at least while still supporting back to node v14 is complex.