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

Add messaging.system tag to all messaging integrations #2590

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

rarguelloF
Copy link
Contributor

@rarguelloF rarguelloF commented Jan 31, 2023

What does this PR do?

Adds messaging.system tag to all messaging integrations:

Integration messaging.system
que que
racecar kafka
resque resque
shoryuken amazonsqs
sneakers rabbitmq
qless qless
delayed_job delayed_job
kafka kafka
sidekiq sidekiq

Motivation

This is part of the Unified Naming Convention initiative.

Additional Notes

How to test the change?

@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Jan 31, 2023
@rarguelloF rarguelloF marked this pull request as ready for review January 31, 2023 18:06
@rarguelloF rarguelloF requested a review from a team January 31, 2023 18:06
Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

Generally fine, but test assertions also should be applied to those failed cases instead of those happy cases.

@@ -21,6 +21,8 @@ module Ext
TAG_OPERATION_ENQUEUE = 'enqueue'.freeze
TAG_OPERATION_JOB = 'job'.freeze
TAG_OPERATION_RESERVE_JOB = 'reserve_job'.freeze

TAG_MESSAGING_SYSTEM = 'delayed_job'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use TAG_COMPONENT instead of introducing an another constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I'll use the TAG_COMPONENT value for integrations where we are not using the message broker itself like kafka, amazonsqs, rabbitmq, etc.

Also I was wondering if there's a good place to put these whitelisted values as mentioned above, that can appear in several integrations, instead of repeating it in the ext.rb file for each one?

@@ -56,6 +56,7 @@
expect(span).to_not have_error
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)).to eq('kafka')
expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_OPERATION)).to eq('connection.request')
expect(span.get_tag(Datadog::Tracing::Contrib::Ext::Messaging::TAG_SYSTEM)).to eq('kafka')
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer plain text to indirection in test

Suggested change
expect(span.get_tag(Datadog::Tracing::Contrib::Ext::Messaging::TAG_SYSTEM)).to eq('kafka')
expect(span.get_tag('messaging.system)).to eq('kafka')

@@ -13,6 +13,8 @@ def redis_info
Datadog::Tracing.trace(Ext::SPAN_REDIS_INFO, service: configuration[:service_name]) do |span|
span.span_type = Datadog::Tracing::Metadata::Ext::AppTypes::TYPE_WORKER

span.set_tag(Contrib::Ext::Messaging::TAG_SYSTEM, Ext::TAG_MESSAGING_SYSTEM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to apply the tag when the instrumented operation isn't really related to push/pop a job from a queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm in this case I'm not sure what's the most correct way to go, since for example if you are using sqs and run the operation ListQueues, which is not a push/pop operation but could be considered as type messaging since you are doing an operation on a messaging system.

So basically this is the logic I followed to tag these operations as messaging (similar logic could be applied for db tags) - also I think making the decision at the integration level makes things easier for future contributors (only need to make these decisions when adding new integrations).

Not a super strong opinion though, so let me know if this makes sense to you.

@rarguelloF rarguelloF force-pushed the rarguelloF/implement-messaging-system-tags branch 2 times, most recently from 472aa6c to f9294bd Compare February 3, 2023 12:58
@rarguelloF rarguelloF force-pushed the rarguelloF/implement-messaging-system-tags branch from f9294bd to 177eb95 Compare February 3, 2023 13:45
Copy link
Contributor

@zarirhamza zarirhamza left a comment

Choose a reason for hiding this comment

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

LGTM

@TonyCTHsu TonyCTHsu merged commit 6b4db20 into master Feb 8, 2023
@TonyCTHsu TonyCTHsu deleted the rarguelloF/implement-messaging-system-tags branch February 8, 2023 10:00
@github-actions github-actions bot added this to the 1.10.0 milestone Feb 8, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants