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

fix(amqplib)!: instrumentation produced high cardinality span names #2366

Merged
merged 11 commits into from
Aug 5, 2024

Conversation

cedricziel
Copy link
Contributor

@cedricziel cedricziel commented Aug 2, 2024

Which problem is this PR solving?

This change amends the amqplib instrumentation so it adheres to span name conventions mentioned here https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#span-name

Ref: #2367

Short description of the changes

Ensure span name is low cardinality and uses a standard operation name

fixes: #2367

This change amends the amqplib instrumentation so it adheres to span name conventions mentioned here https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#span-name
@cedricziel cedricziel changed the title fix: amqplib instrumentation produced high cardinality span names fix(amqplib) instrumentation produced high cardinality span names Aug 2, 2024
@cedricziel cedricziel changed the title fix(amqplib) instrumentation produced high cardinality span names fix(amqplib): instrumentation produced high cardinality span names Aug 2, 2024
@cedricziel cedricziel marked this pull request as ready for review August 2, 2024 09:54
@cedricziel cedricziel requested a review from a team August 2, 2024 09:54
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (dfb2dff) to head (b54f710).
Report is 200 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2366      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.57%     
==========================================
  Files         146      149       +3     
  Lines        7492     7361     -131     
  Branches     1502     1527      +25     
==========================================
- Hits         6816     6655     -161     
- Misses        676      706      +30     
Files Coverage Δ
...lugins/node/instrumentation-amqplib/src/amqplib.ts 90.67% <100.00%> (+0.27%) ⬆️

... and 66 files with indirect coverage changes

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

small nit, otherwise LGTM

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.

LGTM.

I'll wait for a bit to let Amir, as component owner, to have a chance to review.

FWIW, the ... -> ${routingKey} in the span.name dates back to the original commit: aspecto-io/opentelemetry-ext-js@e3bd5ea#diff-f1a7a2a54feb09ebd4bde5c788752e112f0d521fa6de4b05d725a887b2cf743dR305

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

I've not reviewed this in full but it seems like it should be marked as a breaking change, even if it is technically a fix.

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thank you @cedricziel
overall LGTM, left 2 nit suggestions / concerns

plugins/node/instrumentation-amqplib/src/amqplib.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-amqplib/src/amqplib.ts Outdated Show resolved Hide resolved
@cedricziel cedricziel requested a review from blumamir August 5, 2024 10:11
@blumamir
Copy link
Member

blumamir commented Aug 5, 2024

I've not reviewed this in full but it seems like it should be marked as a breaking change, even if it is technically a fix.

Do you think it's breaking because someone might rely on the current name for some logic?
I would personally not consider it breaking, but since the current major is 0 I don't see any harm in marking it as breaking just in case

@blumamir blumamir changed the title fix(amqplib): instrumentation produced high cardinality span names fix(amqplib)!: instrumentation produced high cardinality span names Aug 5, 2024
@blumamir blumamir merged commit 184b19f into open-telemetry:main Aug 5, 2024
22 checks passed
@dyladan dyladan mentioned this pull request Aug 5, 2024
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.

amqplib: span names for publish don't adhere to spec and cause (very) high cardinality
6 participants