chore: fix issue with nyc crashing

[chigia001]

Which problem is this PR solving?

Fixes #1487
Fixes #1594
fixes #1473

Short description of the changes

• nyc is having some hoisting issue with old version of @babel/core => we update the @babel/core package
• update pipeline to exlicit collect code coverage report for node@14 test => this is not behavior change. We just move the control flow to the job's matrix.
• only when need to collect codecov we going to do a full run, and do delta run for other node env (which might have issue with the current --since origin/main logic)

[codecov]

Codecov Report

Merging #1605 (11ba5bf) into main (154b30b) will decrease coverage by 4.29%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1605      +/-   ##
==========================================
- Coverage   96.06%   91.77%   -4.29%     
==========================================
  Files          14      137     +123     
  Lines         914     7065    +6151     
  Branches      199     1424    +1225     
==========================================
+ Hits          878     6484    +5606     
- Misses         36      581     +545     

see 123 files with indirect coverage changes

[chigia001]

With codecov is report correctly for unit test, we miss a bunch of coverage on winston, mongodb as those package include instrumentation code for multiple version.

We can:

• Opt 1: collect code coverage for tav
• Opt 2: begin to ignore coverage for old version?

[chigia001]

@haddasbronfman @dyladan I can confirm @babel/core@7.15.0 is causing the problem, nyc depend on babel to instrument the code

We can use -nohoist="@babel/core" so that lerna install newer version in root or we can update the pinned @babel/core version in our repo like:
https://github.com/chigia001/opentelemetry-js-contrib/blob/531387b5366530e4f1da6c02487bec29bbe89851/metapackages/auto-instrumentations-web/package.json#L36
https://github.com/chigia001/opentelemetry-js-contrib/blob/531387b5366530e4f1da6c02487bec29bbe89851/plugins/web/opentelemetry-instrumentation-document-load/package.json#L53
https://github.com/chigia001/opentelemetry-js-contrib/blob/531387b5366530e4f1da6c02487bec29bbe89851/plugins/web/opentelemetry-instrumentation-long-task/package.json#L50
https://github.com/chigia001/opentelemetry-js-contrib/blob/531387b5366530e4f1da6c02487bec29bbe89851/plugins/web/opentelemetry-plugin-react-load/package.json#L50

[haddasbronfman]

Thanks for handling that and great that you found the issue!! :)

I would pin @babel/core version, rather than using -nohoist="@babel/core"

But I'll wait to see what @dyladan thinks.

[pichlermarc]

Thanks for taking care of this! 🙂 This has been bugging us for quite a while. Glad to see a fix for it. 🚀

With codecov is report correctly for unit test, we miss a bunch of coverage on winston, mongodb as those package include instrumentation code for multiple version.

We can:

• Opt 1: collect code coverage for tav

Ideally, we'd collect coverage for tav. However, tav only runs on PRs which have the package label applied, and then only for the package that the label is applied for. I think codecov would always complain that now coverage is lost when we only run it on main and not PRs. Doing a full run on PRs is quite time-consuming, unfortunately :/

• Opt 2: begin to ignore coverage for old version?

Hmm, do you mean having the new version of the instrumented packages installed as devDependency, and specifically ignoring the instrumentation code intended for older versions? That could be an option. 🤔

@haddasbronfman @dyladan I can confirm @babel/core@7.15.0 is causing the problem, nyc depend on babel to instrument the code

We can use -nohoist="@babel/core" so that lerna install newer version in root or we can update the pinned @babel/core version in our repo like: chigia001/opentelemetry-js-contrib@531387b/metapackages/auto-instrumentations-web/package.json#L36 chigia001/opentelemetry-js-contrib@531387b/plugins/web/opentelemetry-instrumentation-document-load/package.json#L53 chigia001/opentelemetry-js-contrib@531387b/plugins/web/opentelemetry-instrumentation-long-task/package.json#L50 chigia001/opentelemetry-js-contrib@531387b/plugins/web/opentelemetry-plugin-react-load/package.json#L50

Interesting, I've looked into that problem a while ago, but never found a solution. I think I'd prefer we update the pinned version to something newer. 🙂

[dyladan]

Thanks for handling that and great that you found the issue!! :)

I would pin @babel/core version, rather than using -nohoist="@babel/core"

But I'll wait to see what @dyladan thinks.

Sounds good to me as well

[chigia001]

Seem like there is another version mismatch issue with @types/koa and @opentelemetry/instrumentation-aws-sdk

instrumentation-aws-sdk's typescript version is 4.3 but @types/koa@2.13.7 expect to be use with typescript 4.4 as they using Symbol and Template String Pattern Index Signatures

I'm just bit confused why tsc think they need to load @types/koa when compile for instrumentation-aws-sdk

[chigia001]

Hmm, do you mean having the new version of the instrumented packages installed as devDependency, and specifically ignoring the instrumentation code intended for older versions? That could be an option. 🤔

@pichlermarc I mean using using

/* istanbul ignore next */

to skip the whole block of code

[chigia001]

Seem like there is another version mismatch issue with @types/koa and @opentelemetry/instrumentation-aws-sdk

instrumentation-aws-sdk's typescript version is 4.3 but @types/koa@2.13.7 expect to be use with typescript 4.4 as they using Symbol and Template String Pattern Index Signatures

I'm just bit confused why tsc think they need to load @types/koa when compile for instrumentation-aws-sdk

I split this into this PR in case we want to merge this first.

[blumamir]

• using lerna bootstrap --nohoist to force no package hoisting.

Is this still relevant? I see you ended up bumping the version instead of using nohoist.

update pipeline to exlicit collect code coverage report for node@14 test

Just to verify I understand correctly - this is not a behavioral change, right? It was and still running code coverage step only for node 14

[chigia001]

@blumamir

Is this still relevant? I see you ended up bumping the version instead of using nohoist.

No, I update the PR's description to reflect this. The old version is indead use -noHoist as I unable to scope down which package causing the problem.

Just to verify I understand correctly - this is not a behavioral change, right? It was and still running code coverage step only for node 14

Yes, also update PR's description to refelect this.

[blumamir]

LGTM 💯

[blumamir]

seems there is already a new version 7.22.10.

You can optionally bump the version to latest, or we can merge as is and let renovate take care of this

[blumamir]

@chigia001 it is so great you do the research to make codecov stable and correct. Thank you so much for that 🙏

After merging the PR, I checked #1627 and see there are still failures on code-coverage:

Wonder if this can be any help, and if you are aware of more steps we need to take to stabilize the workflow?

[chigia001]

@blumamir I double check 2 latest commit on main and seem like there are still some issue as codecov randomly unable to upload our code coverage file.

[2023-08-10T13:39:52.404Z] ['info'] -> No token specified or token is empty
...
...
...
[2023-08-10T13:39:53.315Z] ['info'] Pinging Codecov: https://codecov.io/upload/v4?package=github-action-3.1.4-uploader-0.6.2&token=*******&branch=main&build=5821384904&build_url=https%3A%2F%2Fgithub.com%2Fopen-telemetry%2Fopentelemetry-js-contrib%2Factions%2Fruns%2F5821384904&commit=4f1124524aee565c3cfbf3975aa5d3d039377621&job=Unit+Tests&pr=&service=github-actions&slug=open-telemetry%2Fopentelemetry-js-contrib&name=&tag=&flags=&parent=
[2023-08-10T13:39:53.315Z] ['verbose'] Passed token was 0 characters long
[2023-08-10T13:39:53.315Z] ['verbose'] https://codecov.io/upload/v4?package=github-action-3.1.4-uploader-0.6.2&branch=main&build=5821384904&build_url=https%3A%2F%2Fgithub.com%2Fopen-telemetry%2Fopentelemetry-js-contrib%2Factions%2Fruns%2F5821384904&commit=4f1124524aee565c3cfbf3975aa5d3d039377621&job=Unit+Tests&pr=&service=github-actions&slug=open-telemetry%2Fopentelemetry-js-contrib&name=&tag=&flags=&parent=
        Content-Type: 'text/plain'
        Content-Encoding: 'gzip'
        X-Reduced-Redundancy: 'false'
[2023-08-10T13:39:53.529Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

This can be fix by adding token key to codecov-action, due to some quota limiting. But this will require change to repo's settings + admin access to codecov's dashboard to get the token => both thing that I'm unable to access, so I can't help on this 😞.

- uses: codecov/codecov-action@v3
  with:
    token: ${{ secrets.CODECOV_TOKEN }}

This comment from codecov-action's repo list out the pro/cons of expose this token value as public variable vs secret:
codecov/codecov-action#557 (comment)

PS: I already put this comment on #1473

PS2: Up-to-date offical response from codecov team: https://community.codecov.com/t/upload-issues-unable-to-locate-build-via-github-actions-api/3954

A malicious actor would be able to upload incorrect or misleading coverage reports to a specific repository if they have access to your upload token, but would not be able to pull down source code or make any code changes.

As there is still some risk associate to this change, maintainer team should discuss about this.

[blumamir]

Thank you so much for the info.
I will bring it up in the next SIG meeting