Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(instrumentation-hapi): use exported strings for attributes #2190

Merged
merged 14 commits into from
May 15, 2024

Conversation

maryliag
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

On package opentelemetry-instrumentation-hapi:

  • Update @opentelemetry/semantic-conventions ^1.23.0
  • Use exported strings for Semantic Attributes.

Use exported strings for Semantic Attributes

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
@maryliag maryliag requested a review from a team May 10, 2024 19:33
@maryliag maryliag changed the title Sem hapi chore(instrumentation-hapi): use exported strings for attributes May 10, 2024
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, what's going on with the 19k line change to package-lock.json?

@maryliag
Copy link
Contributor Author

I hadn't even noticed that... what... how... but... why 😅

@trentm
Copy link
Contributor

trentm commented May 11, 2024

Some of the changes like:

     "node_modules/@aws-crypto/ie11-detection": {
       "version": "2.0.2",
-      "resolved": "https://registry.npmjs.org/@aws-crypto/ie11-detection/-/ie11-detection-2.0.2.tgz",
-      "integrity": "sha512-5XDMQY98gMAf/WRTic5G++jfmS/VLM0rwpiOpaainKi4L0nqWMSB1SzsrEG5rjFZGYN6ZAefO+/Yta2dFM0kMw==",
       "dev": true,
+      "license": "Apache-2.0",
       "dependencies": {
         "tslib": "^1.11.1"
       }
     },

This happens sometimes for reasons I don't yet understand. I am beyond annoyed that npm is so fluid with its package-lock included fields that you can sometimes get monster changes like this. I haven't been able to attribute this inclusion/exclusion of fields like "license", "integrity", "resolved" to a particular npm version, or whether there was a local cache, or the order of running npm ci or npm install or npm update or ... I don't know.

Part of the change updating 1.24.0 to 1.24.1 and 0.51.0 to 0.51.1 are handled in #2180

My total guess is that you ran some sort of npm update or related in your working tree.
I think you'll need to regenerate the package-lock change.

@maryliag
Copy link
Contributor Author

oh yes, I did run npm install after updating the version, my response was more that I didn't expect that amount of change from one version change. I have a feeling no one has run a install or update on this package for awhile.
Something on a new version is breaking one of the tests, so I would also need to check that

Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.12%. Comparing base (dfb2dff) to head (bb3cc46).
Report is 121 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2190      +/-   ##
==========================================
+ Coverage   90.97%   95.12%   +4.14%     
==========================================
  Files         146       10     -136     
  Lines        7492      697    -6795     
  Branches     1502      143    -1359     
==========================================
- Hits         6816      663    -6153     
+ Misses        676       34     -642     

see 137 files with indirect coverage changes

@maryliag
Copy link
Contributor Author

I merged the main branch since the PR you mentioned was merged, and the lock file went form 19k to 17k, which is still a lot, but maybe this is just expected and we should merge it (after I figure out the test failure)

@JamieDanielson
Copy link
Member

I merged the main branch since the PR you mentioned was merged, and the lock file went form 19k to 17k, which is still a lot, but maybe this is just expected and we should merge it (after I figure out the test failure)

I believe the test failure is related to the package-lock change, and if that can get regenerated then the tests will pass.

Locally I pulled down main, went to the hapi directory, and ran npm i @opentelemetry/semantic-conventions@1.22.0 and then npm run compile. Then from the root directory I ran npm ci, npm run compile, and npm run test. Aside from the mongoose test timeout errors I always get locally and the aws test error (new? unrelated and not a problem in CI), other tests passed and my package-lock change is minimal.

@trentm
Copy link
Contributor

trentm commented May 15, 2024

I pushed a small tweak to package-lock to get instr-hapi to use v1.24.1 in the package-lock, even though it is ^1.22.0 in the package.json. I did this by running npm update @opentelemetry/semantic-conventions in the instr-hapi dir.

@trentm trentm enabled auto-merge (squash) May 15, 2024 14:58
@trentm trentm merged commit 5ddb75c into open-telemetry:main May 15, 2024
10 checks passed
@maryliag maryliag deleted the sem-hapi branch May 15, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants