search

rabbitmq / rabbitmq-dotnet-client

RabbitMQ .NET client for .NET Standard 2.0+ and .NET 4.6.2+
https://www.rabbitmq.com/dotnet.html
Other
2.05k stars 575 forks source link

[OTel] Activity is not produced for BasicConsumeAsync on .NET Framework #1533

Closed Kielek closed 4 weeks ago

Kielek commented 2 months ago

Describe the bug

Following code does not generate activity for RabbitMQ.Client.Subscriber activity source.

var consumer = new EventingBasicConsumer(channel);
consumer.Received += (model, ea) =>
{
    var receivedBody = ea.Body.ToArray();
    var receivedMessage = Encoding.UTF8.GetString(receivedBody);
    Console.WriteLine($"Received: {receivedMessage}");
};
channel.BasicConsumeAsync(queue: "hello", autoAck: true, consumer);

Reproduction steps

  1. Have an application consuming messages from queue: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/3373/files#diff-b8bf5e4638613373d455983b52f680cbac4c13732b860567bda571a9e0b29638R40-R44
  2. Have properly defined listener for RabbitMQ.Client.Subscriber
  3. If you are using channel.BasicConsumeAsync on .NET Framework 4.6.2, Activity is not generated for channel.BasicConsumeAsync.

It is working fine for .NET6, .NET7 and .NET8. channel.BasicGetAsync generated proper activities.

Expected behavior

Activity RabbitMQ.Client.Subscriber is generated also for channel.BasicConsumeAsync method on .NET Framework.

Additional context

Found while working on introducing RabbitMQ.Client in OpenTelemetry .NET Automatic Instrumentation.

PR: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/3373

michaelklishin commented 2 months ago

@Kielek thank you for investigating. Have you filed an issue here just for visibility or you expect that in addition to https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/3373, this client's OpenTelemetry integration would also have to change?

If it's the latter, feel free to submit a PR :)

Kielek commented 2 months ago

I would consider this as a bug in RabbitMQ.Client.

I cannot promise that I will have time for fixing it here. Linked PR was the easiest way to provide you application/case which is not working.

lukebakken commented 2 months ago

@stebet is there a chance this issue will be fixed by #1528 ?

lukebakken commented 1 month ago

@Kielek now that #1528 is merged, I will release 7.0.0-RC1. Could you PLEASE test that version to ensure it meets your requirements?

Please note that there will be a RabbitMQ.Client.OpenTelemetry library published as well.

lukebakken commented 1 month ago

@Kielek - 7.0.0-rc.1 is available for you to test with:

Let me know if you have any issues or questions. Thanks!

Kielek commented 4 weeks ago

@lukebakken, I have checked the behavior once more time. The problem was race condition between shouting down event and receiving message event. For some reasons, .NET always handled receiving correctly, .NET Framework proffered shutting down.

I am closing an issues, sorry for bothering you.

lukebakken commented 4 weeks ago

Thank you for following up @Kielek!!!

Kielek commented 4 weeks ago

@lukebakken, one more thing from the code analysis. This line can be removed: https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/3adee90d41106ddc841d9ad66c68e45fc0fd7c84/projects/RabbitMQ.Client.OpenTelemetry/TraceProviderBuilderExtensions.cs#L15

The property is set to true on the RabbitMQActivitySource class.

I have had plan to create PR, but it will take ages to have CLA approval, could you please handle it?

lukebakken commented 4 weeks ago

@Kielek sure thing, here it is - https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1595

Thanks for noticing!