-
Notifications
You must be signed in to change notification settings - Fork 671
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
Fix streak length metric reporting #5172
Fix streak length metric reporting #5172
Conversation
131bbff
to
faca8b9
Compare
@@ -230,7 +230,6 @@ func (p *Propeller) Handle(ctx context.Context, namespace, name string) error { | |||
} | |||
|
|||
streak := 0 | |||
defer p.metrics.StreakLength.Add(ctx, float64(streak)) |
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.
Golang's defer semantics allows us to use this idiom with a slight change. From the spec:
Each time a "defer" statement executes, the function value and parameters to the call are evaluated as usual and saved anew but the actual function is not invoked.
This means that we can define a function that closes over streak
like so:
defer func() { p.metrics.StreakLength.Add(ctx, float64(streak)) }()
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.
Done. As you can probably tell I don't really know what I'm doing with golang 😅
Signed-off-by: Thomas Newton <[email protected]>
Signed-off-by: Thomas Newton <[email protected]>
416c72f
to
a80f1df
Compare
I rebased on latest master, which I'm hoping will fix the docs build error |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5172 +/- ##
==========================================
- Coverage 59.06% 59.06% -0.01%
==========================================
Files 646 646
Lines 55714 55725 +11
==========================================
+ Hits 32910 32915 +5
- Misses 20208 20214 +6
Partials 2596 2596
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Thomas Newton <[email protected]>
a6f2cb9
to
e0a414a
Compare
Pretty sure I fixed the linter error. Initially I struggled a bit to run linting locally. |
* Don't use `defer` for streak length reporting Signed-off-by: Thomas Newton <[email protected]> * Make it work with defer Signed-off-by: Thomas Newton <[email protected]> * Fix lint Signed-off-by: Thomas Newton <[email protected]> --------- Signed-off-by: Thomas Newton <[email protected]>
Tracking issue
Closes #5170
Why are the changes needed?
Accurate metrics are useful.
What changes were proposed in this pull request?
Fix the streak length metric reporting by avoiding use of
defer
. Included slightly simplifying the branching logic.How was this patch tested?
I've been using it in production.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link