Skip to content
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

Adds events for delivered and dropped messages. #1866

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

kallayj
Copy link
Contributor

@kallayj kallayj commented Oct 26, 2023

There is currently neither unit test coverage nor observability of dropped messages due to a full session queue. Unit test coverage and observability are connected by their common use of an event interface provided by MqttServer.

There is also an observability gap of delivered messages. An InterceptingClientEnqueueEvent handler can set AcceptEnqueue to false, so if one wants to, for example, keep track of the total number of delivered messages InterceptingClientEnqueueEvent is not the conceptually correct event to use.

This change is a proposed enhancement to that event interface to cover these gaps.

The proposal here is to add one distinct event for dropped old messages (i.e. due to a MqttPendingMessagesOverflowStrategy of DropOldestQueuedMessage) and another for enqueued or dropped current message:

  1. in the happy case of an enqueued message, a ClientMessageEnqueuedOrDroppedEvent will be raised with a DroppedQueueFull property of false.
  2. in the case of dropping the current message, a ClientMessageEnqueuedOrDroppedEvent will be raised with a DroppedQueueFull property of true.
  3. in the case of an enqueued message with the overwriting of the oldest message in the queue, a QueueMessageOverwrittenEvent will be raised and then a ClientMessageEnqueuedOrDroppedEvent will be raised with a DroppedQueueFull property of false.

The reason to use one event type for enqueued or dropped new messages is to capture the current message context. In the case of a dropped old message we don't have the full context of the message being dropped. An alternative approach would be to have a dropped message event which will be raised when the queue is full using either MqttPendingMessagesOverflowStrategy, and only include the recipient's id on the event (omitting specific message context).

Or...? Suggestions welcomed.

TODO

  • Unit tests covering the two dropped message scenarios

@kallayj
Copy link
Contributor Author

kallayj commented Oct 26, 2023

@dotnet-policy-service agree company="CARIAD Inc."

@CZEMacLeod
Copy link
Contributor

This seems like it would be a good place to start building some metrics for the server, either through OpenTelemetry or similar, and/or by implementing $SYS topics. I think a library to do this stuff would be fun to write and might look at that if this gets implemented.

@kallayj
Copy link
Contributor Author

kallayj commented Oct 26, 2023

@CZEMacLeod we are working from a fork with OpenTelemetry metrics and getting message delivered and dropped metrics is the impetus for this proposed change to the upstream fork.

@kallayj kallayj marked this pull request as ready for review November 6, 2023 18:05
@kallayj
Copy link
Contributor Author

kallayj commented Nov 6, 2023

Converted from a draft to hopefully stimulate feedback from the repo maintainers.

@chkr1011
Copy link
Collaborator

chkr1011 commented Dec 2, 2023

@kallayj I changed some namings so that the new events match the other events in the project.

Please let me know what you think. If there is nothing left I will merge the changes.

@chkr1011
Copy link
Collaborator

chkr1011 commented Dec 2, 2023

This seems like it would be a good place to start building some metrics for the server, either through OpenTelemetry or similar, and/or by implementing $SYS topics. I think a library to do this stuff would be fun to write and might look at that if this gets implemented.

Do you want to start implementing such an extension like "MQTTnet.Extensions.SYS"? I would support that by adding more events and APIs that are needed to obtain the required data.

@kallayj
Copy link
Contributor Author

kallayj commented Dec 4, 2023

@chkr1011 your changes look good to me.

See #1867 for my proposal for leveraging the events to start publishing metrics.

I would order using the conventional metrics publishing mechanism ahead of publishing them on $SYS topics.

@chkr1011
Copy link
Collaborator

chkr1011 commented Dec 6, 2023

The metrics extensions look OK to me but we should also start supporting the $SYS topics as well because several MQTT clients are using them for statistics etc. But that is probably another PR.

@chkr1011 chkr1011 merged commit 09ab30f into dotnet:master Dec 6, 2023
1 of 2 checks passed
kallayj added a commit to kallayj/MQTTnet that referenced this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants