Feedback on 7 rc.11

[ngbrown]

@lukebakken had asked me for feedback on the v7 RC, so this is my initial feedback.

I updated the rabbit-rpc branch on the repo here: https://github.com/ngbrown/grpc-testing/commits/rabbit-rpc/, so look at the recent commits.

Describe the solution you'd like

• The conversion of methods to Async went ok because the method names changed. If there are any that the name didn't change and also converted to async, then I probably missed them.
• I'm not clear on what value of mandatory is equivalent to the v6 behavior. The parameter also isn't optional, at least on the BasicPublishAsync method I use. This parameter should have a default value for easier migration. Or at least mention in the Doc XML what the equivalent value is for v6 behavior.

Additional context

I also plan on testing how System.Diagnostics.Activity works with AsyncEventingBasicConsumer.Received event handlers, but I won't be able to look at that until the end of the week, so thought that I would get this feedback out there.

[michaelklishin]

We should make mandatory default to false, it is the case in other clients and in general, mandatory publishing is not what the majority of users need, most of the time.

[lukebakken]

I'm not clear on what value of mandatory is equivalent to the v6 behavior. The parameter also isn't optional, at least on the BasicPublishAsync method I use.

Which method do you use? Can you point it out in the 6.x code? I'm guessing it is one of these:

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/6.x/projects/RabbitMQ.Client/client/api/IModelExtensions.cs#L77-L105

[lukebakken]

Unfortunately I can't seem to restore the v6 behavior without causing "ambiguous reference" errors in the code.

[ngbrown]

In v6, I was using:

this._channel.BasicPublish(exchange: this._replyExchangeName, routingKey: this._props.ReplyTo, basicProperties: replyProps, body: responseBytes)

[lukebakken]

Thanks, I'll check again but there's a good chance we can't support this particular overload.

[ngbrown]

So all my calls to BasicPublish went to

rabbitmq-dotnet-client/projects/RabbitMQ.Client/client/api/IModelExtensions.cs

Line 94 in 86a7b0f

public static void BasicPublish(this IModel model, string exchange, string routingKey, IBasicProperties basicProperties, ReadOnlyMemory<byte> body)

I see by looking at the other overloads in v6 that mandatory = false was the default. My suggestion would be to at least add that default information to the Doc XML.

[ngbrown]

I started working on integrating with the ActivitySource addition in the v7 RabbitMQ Client. There doesn't appear to be any DiagnosticListener I can subscribe to. I'm using SerilogTracing and it would be ideal to be able to subscribe to the RabbitMQ activities and generate my own templated strings that are not unique on every activity. This would eventually be rolled into a library like is available here.

I'm getting the activities from RabbitMQ fine, but the only two diagnostic sources are "Microsoft.Extensions.Hosting" and "Microsoft.AspNetCore".

From the Activity User Guide:

DO - create new DiagnosticListener for specific Activity type to allow filtering by activity. E.g Incoming and Outgoing Http Requests should be different DiagnosticListeners. Follow DiagnosticSource User Guide to pick a name.

I should be able to filter on the RabbitMQ activities by using the DiagnosticListener.AllListeners.Subscribe API.

[lukebakken]

There doesn't appear to be any DiagnosticListener I can subscribe to

Is this something that exists in the 6.x version of this client?

[ngbrown]

No, this is a new feature of the v7 client via #1261.

[lukebakken]

Ah, gotcha. Well, we've only used ActivitySource to add support for OpenTelemetry, and to be honest, I'm not very familiar with how that works.

Maybe @stebet can chime in?