-
Notifications
You must be signed in to change notification settings - Fork 56
Tracing: add useful tags to Trigger spans #253
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
base: dev
Are you sure you want to change the base?
Conversation
hello @vivekjilla @aloiva @manikantanallagatla could you take a look at this pull request, please? thanks! |
Hi @aaguilartablada thank you for your contribution. Could you provide a little more description and use cases? Curious to know how this change effects telemetry. |
Of course. Here you can see a trace from Grafana Tempo: As you can see, an application publishes a message and another application using this library consumes the message generating a "Trigger" span. The point is that "Span Attributes" is empty. The idea of this pull request is filling span attributes with some useful information. |
@@ -107,6 +107,9 @@ public Task StartAsync(CancellationToken cancellationToken) | |||
async Task ReceivedHandler(object model, BasicDeliverEventArgs args) | |||
{ | |||
using Activity activity = RabbitMQActivitySource.StartActivity(args.BasicProperties); | |||
activity?.AddTag("amqp.queue", this.queue); | |||
activity?.AddTag("amqp.channel", this.channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.channel
is not a string so it cannot be used here. maybe we can have args.DeliveryTag
instead.
@@ -118,6 +121,7 @@ async Task ReceivedHandler(object model, BasicDeliverEventArgs args) | |||
args.BasicProperties.Headers ??= new Dictionary<string, object>(); | |||
args.BasicProperties.Headers.TryGetValue(RequeueCountHeaderName, out object headerValue); | |||
int requeueCount = Convert.ToInt32(headerValue, CultureInfo.InvariantCulture) + 1; | |||
activity?.AddTag("amqp.requeueCount", requeueCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds vague since this would show requeueCount=1 even when the is message not requeued. i suggest modifying to
activity?.AddTag("amqp.currentRequeueCount", requeueCount-1);
Spans generated from this library don't have any tags to identify the source queue and other useful information. With this PR 4 tags are included: