-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
output/json: Set and flush thresholds #1886
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1886 +/- ##
==========================================
+ Coverage 71.16% 71.21% +0.05%
==========================================
Files 183 183
Lines 14325 14323 -2
==========================================
+ Hits 10194 10200 +6
+ Misses 3505 3497 -8
Partials 626 626
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM!
Can you please rebase the PR on top of the latest master
, the new CI check we added on Friday (#1885) is failing the build, because GitHub runs the CI on top an imaginary merge commit between your branch and master, but the CI check uses your actual PR commits (because it essentially does go get
).
@na-- I rebased, but when I opened the PR I was already based on the latest version. I forked on Saturday. Can the check work with a fork? Because from my understanding it's trying to exec a |
Ah, yeah, sorry... 😞 I created #1891 and we'll fix it soon after we release v0.31.0, which would hopefully happen tomorrow. The |
@na-- Sorry, I forgot to mention that the fix is already included in the RS and pushed. |
The JSON Output implements WithThresholds so the Engine can set the thresholds invoking SetThresholds. The flush logic sets the Metric's Thresholds to write them in the json output. Closes #1052
@na-- Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Seems like the check is now ok 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your contribution!
Added the ability to set and flush the thresholds implementing the WithThresholds interface as required from the Engine.
The SetThresholds implementation is the same as the cloud version. If you like the idea, I could also refactor out it to centralize the logic.
Closes #1052