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.

Sign up for GitHub

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

Jump to bottom

[#3520] specific metrics for unknown messages #3519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[#3520] specific metrics for unknown messages #3519

wants to merge 1 commit into from
+846 −77

Conversation

BobClaerhout
Copy link
Contributor

@BobClaerhout BobClaerhout commented Jul 24, 2023 •
edited
Loading

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

@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 force-pushed the specificMetricsForUnknownMessages branch 4 times, most recently from 8bab1a5 to 39e59a0 Compare July 25, 2023 15:35
sophokles73
sophokles73 reviewed Jul 27, 2023
View reviewed changes
service-base/src/main/java/org/eclipse/hono/service/metric/MetricsTags.java Outdated
Comment on lines 255 to 267
/**
* The message received has a bad syntax and could not be parsed.
*/
BAD_SYNTAX("bad-syntax"),
/**
* The type of the message could not be deducted from the message.
*/
UNKNOWN_TYPE("unknown-type"),
/**
* The type of the message is not supported for this action.
*/
UNSUPPORTED_TYPE("unsupported-type"),
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.

@calohmn calohmn mentioned this pull request Aug 12, 2023
Open
@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?

@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
Reviewers

@calohmn calohmn calohmn left review comments

@sophokles73 sophokles73 sophokles73 left review comments

@sonatype-lift sonatype-lift sonatype-lift left review comments

@dejanb dejanb Awaiting requested review from dejanb dejanb is a code owner

@kaniyan kaniyan Awaiting requested review from kaniyan kaniyan is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BobClaerhout @sophokles73 @calohmn