RabbitMQActivitySource.Deliver cannot be used by types that implement Consumer

[Tornhoof]

Describe the bug

Currently RabbitMQActivitySource.Deliver is only called in the derived types AsyncEventingBasicConsumer and EventingBasicConsumer and the method itself is marked as internal.

rabbitmq-dotnet-client/projects/RabbitMQ.Client/client/events/AsyncEventingBasicConsumer.cs

Lines 99 to 105 in 89d472d

	private async Task BasicDeliverWrapper(BasicDeliverEventArgs eventArgs)
	{
	using (Activity activity = RabbitMQActivitySource.Deliver(eventArgs))
	{
	await _receivedWrapper.InvokeAsync(this, eventArgs).ConfigureAwait(false);
	}
	}

This means, if a dev is deriving from the base types DefaultBasicConsumer or AsyncDefaultBasicConsumer or implements the appropriate interfaces, the Deliver method can't be invoked.

All the other relevant ActivitySource methods are called from within ChannelBase, so these should be fine.

I think the easiest way forward is to make a public RabbitMQActivitySource.Deliver method with the args from the eventArgs:

Current:

rabbitmq-dotnet-client/projects/RabbitMQ.Client/client/impl/RabbitMQActivitySource.cs

Lines 122 to 142 in 89d472d

	internal static Activity Deliver(BasicDeliverEventArgs deliverEventArgs)
	{
	if (!s_subscriberSource.HasListeners())
	{
	return null;
	}
	// Extract the PropagationContext of the upstream parent from the message headers.
	Activity activity = s_subscriberSource.StartLinkedRabbitMQActivity(
	UseRoutingKeyAsOperationName ? $"{deliverEventArgs.RoutingKey} deliver" : "deliver",
	ActivityKind.Consumer, ContextExtractor(deliverEventArgs.BasicProperties));
	if (activity != null && activity.IsAllDataRequested)
	{
	PopulateMessagingTags("deliver", deliverEventArgs.RoutingKey, deliverEventArgs.Exchange,
	deliverEventArgs.DeliveryTag, deliverEventArgs.BasicProperties, deliverEventArgs.Body.Length,
	activity);
	}
	return activity;
	}

Proposed change:

        internal static Activity Deliver(BasicDeliverEventArgs deliverEventArgs)
        {
            return Deliver(deliverEventArgs.RoutingKey, deliverEventArgs.Exchange, deliverEventArgs.DeliveryTag,
                deliverEventArgs.BasicProperties, deliverEventArgs.Body.Length);
        }

        public static Activity Deliver(string routingKey, string exchange, ulong deliveryTag,
            in ReadOnlyBasicProperties readOnlyBasicProperties, int bodySize)
        {
            if (!s_subscriberSource.HasListeners())
            {
                return null;
            }

            // Extract the PropagationContext of the upstream parent from the message headers.
            Activity activity = s_subscriberSource.StartLinkedRabbitMQActivity(
                UseRoutingKeyAsOperationName ? $"{routingKey} deliver" : "deliver",
                ActivityKind.Consumer, ContextExtractor(readOnlyBasicProperties));
            if (activity != null && activity.IsAllDataRequested)
            {
                PopulateMessagingTags("deliver", routingKey, exchange,
                    deliveryTag, readOnlyBasicProperties, bodySize,
                    activity);
            }

            return activity;
        }

Then the dev can call it in their own implementation of HandleBasicDeliver. I don't see any easy way to do it automatically, like for the other methods, that would be preferable.

Reproduction steps

Expected behavior

Additional context

Maybe @stebet has a better idea, he wrote the original PR.

[stebet]

I'll take a look :)

[lukebakken]

@stebet thank you!!!

[bollhals]

Wouldn't it be better to move it inside of the dispatcher rather than the BasicConsumer?

[lukebakken]

@bollhals would you mind expanding on your comment? I'm assuming that you mean RabbitMQActivitySource.Deliver should be called in these places:

• https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/client/impl/ConsumerDispatching/AsyncConsumerDispatcher.cs#L29-L31
• https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/client/impl/ConsumerDispatching/ConsumerDispatcher.cs#L31-L35

[bollhals]

Yes exactly. I wondered whether it would make more sense to do it at that level than in the consumer themselves.

[lukebakken]

@stebet @Tornhoof @bollhals -> #1633

Let me know what you think!

[bollhals]

looks reasonable to me.

[Tornhoof]

Yeah, I agree, this looks quite neat.