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: change span names for socket-io #2495

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

povilasv
Copy link
Contributor

@povilasv povilasv commented Oct 21, 2024

Which problem is this PR solving?

Socket io span names might contain high cardinality data. I'm removing roomId from span names so it doesn't.

Issue: #1937

Short description of the changes

  • I refactored names so It follows messaging semconv, it's "send ${namespace}" / "receive ${namespace}" instead of ... receive.

One question about should it be "receive", I think we might need to change it to "process"?

Given:

“Receive” spans SHOULD be created for operations of passing messages to the application when those operations are initiated by the application code (pull-based scenarios).

“Process” spans SHOULD be created for operations of passing messages to the application when those operations are not initiated by the application code (push-based scenarios). Such “Process” span covers the duration of such an operation, which is usually a callback or handler.

Ref: https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#consumer-spans

@povilasv povilasv marked this pull request as ready for review October 21, 2024 11:06
@povilasv povilasv requested a review from a team as a code owner October 21, 2024 11:06
@github-actions github-actions bot requested a review from mottibec October 21, 2024 11:10
@povilasv povilasv force-pushed the socket-io-span-name branch from 8b56ce9 to 17140b4 Compare October 21, 2024 11:27
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.88%. Comparing base (30e8fc5) to head (05712c6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2495   +/-   ##
=======================================
  Coverage   90.88%   90.88%           
=======================================
  Files         159      159           
  Lines        7844     7844           
  Branches     1616     1616           
=======================================
  Hits         7129     7129           
  Misses        715      715           

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.

LGTM, the room should not be part of the span name and the operation should be first according to the span name of messaging spans.

Thanks for fixing it.

@blumamir
Copy link
Member

@povilasv please fix the lint error with npm run lint so CI is green

@povilasv povilasv force-pushed the socket-io-span-name branch from 17140b4 to eff2981 Compare October 23, 2024 09:27
@povilasv povilasv force-pushed the socket-io-span-name branch from eff2981 to 05712c6 Compare October 23, 2024 09:29
@povilasv
Copy link
Contributor Author

@blumamir fixed, thanks!

@pichlermarc pichlermarc merged commit 86dba74 into open-telemetry:main Oct 23, 2024
23 checks passed
@dyladan dyladan mentioned this pull request Oct 23, 2024
@povilasv povilasv deleted the socket-io-span-name branch October 24, 2024 11:54
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.

4 participants