Performance counters (metrics)

[bollhals]

Proposed Changes

This change introduces performance counters for the client.
For now available are:

• total-connection-opened
• open-connection-count
• total-channel-opened
• open-channel-count
• bytes-sent-rate
• bytes-received-rate
• AMQP-method-sent-rate
• AMQP-method-received-rate

fixes the counter part of #776

I've also fixed in the first commit a bug that happens if the first read into the header buffer returned not the full 7 bytes. (happend a lot when I ran the MassPublish test app with 50 mio messages.)

Types of Changes

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

[bollhals]

Do we want other counters or are we fine with the current ones for now?

Example output for dotnet-counters

[bollhals]

I changed it to internal, as others shouldn't be able to do anything with these methods here

[michaelklishin]

Should the totals be named in plural, e.g.

• Total connections opened
• Total channels opened
• Currently open connection count
• Currently open channel count

We can also use the terms from the Prometheus/OpenTelemetry communities, e.g. gauge and counter for suffixes.

[bollhals]

Should the totals be named in plural, e.g.

• Total connections opened
• Total channels opened
• Currently open connection count
• Currently open channel count

We can also use the terms from the Prometheus/OpenTelemetry communities, e.g. gauge and counter for suffixes.

Happy to adapt to whatever we agree upon 👍
We should probably also consider what already exist in the dotnet sources

[michaelklishin]

Looking at those search results I'd say the names should be

• Total connections opened
• Total channels opened
• Current connection counter
• Current channel counter

This would very much follow the names in KestrelEventSource.cs, for example, but "opened" would be slightly clearer. We can also add a "closed" counterpart so it would be easy to see if there are any connection or channel leaks. "Current" counters do not necessarily make it obvious.

[bollhals]

I used to have the closed counterpart, but the thing is you always want to know how many are still open, so you have to compare agsinst the open counter. => just have an open/current counter.
I assume the names eould be the same but the spaces replaced with dashes? (Need both name & displayname)

[stebet]

This is awesome!

I'd love to see frames sent/received if that's not too hard to add as well.

[michaelklishin]

Yes, names can be underscored or dashed as conventional in this particular community.

[bollhals]

This is awesome!

I'd love to see frames sent/received if that's not too hard to add as well.

Frames received is easy, sent not really as we combine the three frames of a basic deliver method into one byte blob to send. (See the bytes/command sent, they're done in the same place)
OR we'd have to count them before we actually sent them, but this is kind of lying as it wasn't sent yet at that time.

And only having the frames received is also strange, that's why I havet't added them in this first draft.

Do you have an idea on how we could solve this best?

[bollhals]

Updated the names to

• total-connections-opened
• current-open-connections
• total-channels-opened
• current-open-channels

Also updated the event source name to rabbitmq-client

[michaelklishin]

@bollhals can you please rebase this on top of master? Then we can merge.

[bollhals]

@bollhals can you please rebase this on top of master? Then we can merge.

done, when the build is still successful, we can merge.

[xsoheilalizadeh]

I'm not able to read these metrics, by dotnet monitor. I'm using Masstranist with Rabbitmq transport, I tried rabbitmq-client and rabbitmq-dotnet-client for the counter name but It didn't work out. Does anyone have any idea how can I enable this?

[bollhals]

I'm not sure whether this is in the 6.x branch or is yet to be released.

[xsoheilalizadeh]

@bollhals When this is going to be merged or released?

[lukebakken]

@xsoheilalizadeh - I suggest checking out the master branch and compiling this library yourself. We didn't plan to back-port this to 6.x, but I could do that.

@Gsantomaggio @michaelklishin back port?

[xsoheilalizadeh]

Unfortunately, that's not an option for me.

[michaelklishin]

@lukebakken given that this is a feature and not a bug, maybe we should ship 7.0 sooner. I wouldn't be against backporting but again, it is not a bug fix and this community cares a lot about us following the strict interpretation of SemVer.

[bollhals]

Up to you guys. Irrespective pf this, I'd advise to start a thread about 7.0 and the remaining work so anyone has the same understanding and the community knows what/when to expect.

[lukebakken]

@bollhals please discuss here - #1191

[xsoheilalizadeh]

Do you have any ETA for 7.0 release?

[lukebakken]

No we do not.

[eerhardt]

Also updated the event source name to rabbitmq-client

@bollhals @lukebakken @michaelklishin - is there a reason the event source name was changed from rabbitmq-dotnet-client to rabbitmq-client? This is a breaking change for anyone who is listening to these EventSource events when moving from 6.x to 7.0.

[michaelklishin]

@eerhardt having "dotnet" in the name seems redundant. It was introduced for the next major version, I don't think a breaking change in a new major should generally be considered controversial.

[lukebakken]

@eerhardt my guess is that since the main namespace for this client is RabbitMQ.Client, that change was made to bring it in line.