Allow per-deployment metrics check interval #39
Conversation
vbehar
requested review from
jeremyrickard,
krancour and
yaron2
as code owners
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 57.3% 59.09% +1.78%
==========================================
Files 11 11
Lines 623 638 +15
==========================================
+ Hits 357 377 +20
+ Misses 237 232 -5
Partials 29 29
Continue to review full report at Codecov.
|
Add support for a new annotation `osiris.deislabs.io/metricsCheckInterval` on deployments, to override the default metricsCheckInterval on a per-deployment basis. I kept the same name and unit (seconds) to be consistent.
vbehar
force-pushed
the
per-deployment-metrics-check-interval
branch
from
a646257 to
3eca5e4
Compare
| metricsCheckInterval = z.cfg.MetricsCheckInterval | ||
| } | ||
| if metricsCheckInterval <= 0 { | ||
| metricsCheckInterval = z.cfg.MetricsCheckInterval |
There was a problem hiding this comment.
This is probably worthy of a warning.
| deployment.Name, | ||
| z.cfg.MetricsCheckInterval, | ||
| err, | ||
| ) |
There was a problem hiding this comment.
The error you return from GetMetricsCheckInterval() already indicates it was an issue with paring the value. Since you're including the text of the error at the end of the warning message, this information basically appears twice.
I would refrain from mentioning the value as "invalid" here and just note "there was an error getting.... falling back to." This way, also, if GetMetricsCheckInterval() ever changes and produces other possible errors as well, there won't be text elsewhere in the code (e.g. right here) that needs to be adjusted to account for the different sorts of errors that function can produce.
pkg/kubernetes/osiris_test.go
Outdated
| t.Run(test.name, func(t *testing.T) { | ||
| actual, err := GetMetricsCheckInterval(test.annotations) | ||
| if err != nil { | ||
| if !test.expectedError { |
There was a problem hiding this comment.
Would you mind using testify for assertions? I believe that is used fairly consistently across the other unit tests (few though they may admittedly be).
vbehar
force-pushed
the
per-deployment-metrics-check-interval
branch
from
2acf827 to
1ddfe31
Compare
|
LGTM. Thanks @vbehar! |
* Allow per-deployment metrics check interval Add support for a new annotation `osiris.deislabs.io/metricsCheckInterval` on deployments, to override the default metricsCheckInterval on a per-deployment basis. I kept the same name and unit (seconds) to be consistent. * expose error from parsing annotation value * add documentation * add a warning if metricsCheckInterval is invalid * improve warning message * switch to testify for tests
backport deislabs#39 Add support for a new annotation `osiris.deislabs.io/metricsCheckInterval` on deployments, to override the default metricsCheckInterval on a per-deployment basis. I kept the same name and unit (seconds) to be consistent.
3 participants
Add support for a new annotation
osiris.deislabs.io/metricsCheckIntervalon deployments,to override the default metricsCheckInterval on a per-deployment basis.
I kept the same name and unit (seconds) to be consistent.