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

refactor(instr-cassandra-driver): use exported strings for attributes #2139

Merged

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Update @opentelemetry/semantic-conventions to ^1.22
  • Replace SemanticAttributes.* with exported strings SEMATTRS_*
  • Update README with new semantic convention package version and key

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.47%. Comparing base (dfb2dff) to head (5c159e8).
Report is 97 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2139      +/-   ##
==========================================
- Coverage   90.97%   90.47%   -0.50%     
==========================================
  Files         146      147       +1     
  Lines        7492     7594     +102     
  Branches     1502     1575      +73     
==========================================
+ Hits         6816     6871      +55     
- Misses        676      723      +47     
Files Coverage Δ
...y-instrumentation-cassandra/src/instrumentation.ts 81.76% <50.00%> (+0.36%) ⬆️

... and 29 files with indirect coverage changes

@blumamir
Copy link
Member

this is ready to merge, but for some reason the TAV test fails with this message:

Run npm run --if-present --workspaces test-all-versions -w @opentelemetry/instrumentation-cassandra
npm ERR! No workspaces found:
npm ERR!   --workspace=@opentelemetry/instrumentation-cassandra

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2024-04-[29](https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/8874128142/job/24361069901?pr=2139#step:9:30)T06_22_44_086Z-debug-0.log
Error: Process completed with exit code 1.

The changes seems unrelated, but the error does mention "npm ERR! --workspace=@opentelemetry/instrumentation-cassandra" so I guess something is broken in the build?
any ideas?

@JamieDanielson
Copy link
Member

Hm it looks like cassandra is one of the few instrumentations that doesn't have a "tav" script so I guess it should be skipped in this test? What's weird though is I would've expected #2099 to fail too then because it also doesn't have tav. 🤔

@trentm
Copy link
Contributor

trentm commented May 1, 2024

What's weird though is I would've expected #2099 to fail too then because it also doesn't have tav. 🤔

The TAV workflow didn't run on that PR. For some reason the "TAV for PR" didn't trigger. I don't know why. Hrm.
However, even if it had, it would have passed:

% npm run --if-present --workspaces test-all-versions -w @opentelemetry/instrumentation-router
%

The issue with instr-cassandra is confusion with the dir name and issue label using "instrumentation-cassandra", but the package actually being instrumentation-cassandra-driver.

If there are multiple workspace args, then I think it all works:

% npm run --if-present --workspaces test-all-versions -w @opentelemetry/instrumentation-cassandra -w @opentelemetry/instrumentation-hapi

> @opentelemetry/instrumentation-hapi@0.37.0 test-all-versions
> tav
...

So this will only happen on a PR that only has the pkg:instr-cassandra label -- which hasn't happened before (https://github.com/open-telemetry/opentelemetry-js-contrib/issues?q=label%3Apkg%3Ainstrumentation-cassandra+).

fix

I'll try to fix with:

  1. This patch:
--- a/.github/component-label-map.yml
+++ b/.github/component-label-map.yml
@@ -71,7 +71,7 @@ pkg:instrumentation-bunyan:
   - changed-files:
       - any-glob-to-any-file:
           - plugins/node/opentelemetry-instrumentation-bunyan/**
-pkg:instrumentation-cassandra:
+pkg:instrumentation-cassandra-driver:
  1. and changing the label name to have "-driver".

@trentm trentm changed the title refactor(instr-cassandra): use exported strings for attributes refactor(instr-cassandra-driver): use exported strings for attributes May 1, 2024
@trentm
Copy link
Contributor

trentm commented May 1, 2024

2. and changing the label name to have "-driver".

Oh, there are already both label names: https://github.com/open-telemetry/opentelemetry-js-contrib/labels?q=cassandra

Screenshot 2024-04-30 at 7 15 41 PM

I'll update all to the one we want and delete the other package label.

@trentm
Copy link
Contributor

trentm commented May 1, 2024

delete the other package label.

We'll have to wait to delete the other package label until after this change goes in, because the github action adding component labels is re-adding it.

@trentm trentm merged commit e74cee4 into open-telemetry:main May 1, 2024
16 of 17 checks passed
@david-luna david-luna deleted the dluna/cassandra-semconv-strings branch May 2, 2024 14:06
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.

None yet

5 participants