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

Allow Ridley to survive exceptions thrown by handlers #31

Merged
merged 1 commit into from
Mar 16, 2022
Merged

Allow Ridley to survive exceptions thrown by handlers #31

merged 1 commit into from
Mar 16, 2022

Conversation

adinapoli
Copy link
Contributor

Fixes #30.

I have taken the liberty of reshuffling things around a bit, and notably we now have a new smart constructor mkRidleyMetricHandler which will automatically populate a label field with the CallStack in scope, so that users will get a somewhat precise information of which handler errored out:

Screenshot 2022-03-10 at 16 39 50

@adinapoli
Copy link
Contributor Author

@adamgundry If you could put your stamp of approval on this, it would be appreciated as always 😉

@adinapoli adinapoli force-pushed the adinapoli/issue-30 branch 3 times, most recently from 117744f to 1e7e4fe Compare March 15, 2022 10:42
@adinapoli
Copy link
Contributor Author

@adamgundry I have made some changes as per our previous discussion and now I have embedded the CallStack as a separate field. The RidleyMetricHandler type has now been moved to an .Internal module, to encourage the use of the smart constructor mkRidleyHandler. When we display the error, we now print both the label and the callstack:

Screenshot 2022-03-15 at 11 08 38

adamgundry
adamgundry reviewed Mar 15, 2022
View reviewed changes
ridley/src/System/Metrics/Prometheus/Ridley.hs Outdated
Comment on lines 215 to 218
fromString $ "Couldn't update handler for "
<> "\"" <> T.unpack lbl <> "\""
<> " originally defined at "
<> prettyCallStack cs
<> " due to "
<> Ex.displayException ex
Copy link

Choose a reason for hiding this comment

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

I (still) think the message should have the exception before the call stack (because the latter breaks over multiple lines). Otherwise this looks reasonable, though I'm not familiar with the guts of ridley.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, I will swap the order and merge this one. Thanks! 😸

@adinapoli adinapoli force-pushed the adinapoli/issue-30 branch from 1e7e4fe to f3b1741 Compare March 16, 2022 11:19
@adinapoli adinapoli merged commit 94c03a6 into master Mar 16, 2022
@adinapoli adinapoli deleted the adinapoli/issue-30 branch March 16, 2022 11:22
@adamgundry adamgundry mentioned this pull request Aug 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamgundry adamgundry adamgundry left review comments

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

Successfully merging this pull request may close these issues.

Catch exceptions thrown by metric handlers
2 participants
@adinapoli @adamgundry