Skip to content

[#3520] specific metrics for unknown messages#3519

Open
BobClaerhout wants to merge 1 commit intoeclipse-hono:masterfrom
BobClaerhout:specificMetricsForUnknownMessages
Open

[#3520] specific metrics for unknown messages#3519
BobClaerhout wants to merge 1 commit intoeclipse-hono:masterfrom
BobClaerhout:specificMetricsForUnknownMessages

Conversation

@BobClaerhout
Copy link
Contributor

@BobClaerhout BobClaerhout commented Jul 24, 2023

We would like to have a different metrics for unknown messages compared to messages which are processable (for some reason). This PR adds this metric for the unknown messages

@BobClaerhout BobClaerhout requested a review from calohmn as a code owner July 24, 2023 15:11
@sonatype-lift
Copy link

sonatype-lift bot commented Jul 24, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@sophokles73
Copy link
Contributor

@BobClaerhout would you mind creating an issue for this feature request which we could use to discuss? It is not obvious to me what the motivation for this change is so providing a little context in the issue would be very helpful (to me :-)).

@BobClaerhout BobClaerhout changed the title specific metrics for unknown messages [#3520] specific metrics for unknown messages Jul 25, 2023
@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch from 1bb6c34 to 00f5993 Compare July 25, 2023 07:48
@BobClaerhout
Copy link
Contributor Author

@BobClaerhout would you mind creating an issue for this feature request which we could use to discuss? It is not obvious to me what the motivation for this change is so providing a little context in the issue would be very helpful (to me :-)).

Created a ticket: #3520

@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch from 00f5993 to 51251a6 Compare July 25, 2023 08:34
@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch 2 times, most recently from 18bf45a to 6ac3f87 Compare July 25, 2023 09:35
@sonatype-lift
Copy link

sonatype-lift bot commented Jul 25, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/eclipse-hono/hono/3519.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/eclipse-hono/hono/3519.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch 3 times, most recently from ff18da6 to 84c0094 Compare July 25, 2023 15:12
@BobClaerhout BobClaerhout requested a review from kaniyan as a code owner July 25, 2023 15:12
@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch 4 times, most recently from 8bab1a5 to 39e59a0 Compare July 25, 2023 15:35
Comment on lines 255 to 267
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do these reasons have any meaning outside the scope of the Lora adapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the first 3 do not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see any particular value in reporting the generic reasons as well? We do have the reason for failed message processing in the traces and so far, nobody needed to have them reported in metrics.
If we do not need them then I would suggest to only define and use the Lora specific reasons in the context of the Lora adpter in order not to mix up generic and adapter specific error handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the others are useful as well. It provides a lot more insight into missed messages. Lot's of our devices are out in the field an unreachable. Having some reporting from the cloud services makes sense imo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only concern I have is that these kinds of issues (currently) only occur with the Lora adapter because none of the other adapters actually care about the message payload. In fact, being payload agnostic is actually one of Hono's core features/assertions ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but it is very useful information. If a message is unprocessable because it was wrongly formatted, it's completely different from a unknown type. From an operational standpoint, we need to act differently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't hurt, right? It only adds information in the metrics. You are free to use it or not.

@sophokles73
Copy link
Contributor

@BobClaerhout any update on this or can we defer this to after the 2.4.0 release?

@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch 2 times, most recently from 5852676 to 859f41d Compare August 24, 2023 10:04
@BobClaerhout
Copy link
Contributor Author

@BobClaerhout any update on this or can we defer this to after the 2.4.0 release?

Sorry for the delay. I've applied all remarks from @calohmn. Release 2.4.0 is just fine for me

@BobClaerhout
Copy link
Contributor Author

@calohmn , @sophokles73 , any other remarks?

Signed-off-by: Bob Claerhout <claerhout.bob@gmail.com>
@BobClaerhout BobClaerhout force-pushed the specificMetricsForUnknownMessages branch from 859f41d to e94a14c Compare September 12, 2023 13:12
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