Add messaging.system tag to all messaging integrations#2590
Conversation
TonyCTHsu
left a comment
TonyCTHsu
left a comment
There was a problem hiding this comment.
Generally fine, but test assertions also should be applied to those failed cases instead of those happy cases.
| 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?
| 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') |
| 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
3 participants
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?