Add messaging.system tag to all messaging integrations #2590
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
I think we could use TAG_COMPONENT instead of introducing an another constant.
There was a problem hiding this comment.
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') | |||
There was a problem hiding this comment.
Prefer plain text to indirection in test
| 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) | |||
There was a problem hiding this comment.
Does it make sense to apply the tag when the instrumented operation isn't really related to push/pop a job from a queue?
There was a problem hiding this comment.
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
force-pushed
the
rarguelloF/implement-messaging-system-tags
branch
2 times, most recently
from
472aa6c
to
f9294bd
Compare
rarguelloF
force-pushed
the
rarguelloF/implement-messaging-system-tags
branch
from
f9294bd
to
177eb95
Compare
What does this PR do?
Adds messaging.system tag to all messaging integrations:
quequeracecarkafkaresqueresqueshoryukenamazonsqssneakersrabbitmqqlessqlessdelayed_jobdelayed_jobkafkakafkasidekiqsidekiqMotivation
This is part of the Unified Naming Convention initiative.
Additional Notes
How to test the change?