Fix metrics reporting period #14019
Fix metrics reporting period #14019
Conversation
knative-prow
bot
added
area/API
knative-prow
bot
added
the
approved
|
/assign @dprotaso |
| metrics.BackendDestinationKey: backend, | ||
| "metrics.opencensus-address": collectorAddress, | ||
| metrics.BackendDestinationKey: backend, | ||
| "metrics.opencensus-address": collectorAddress, |
There was a problem hiding this comment.
We need to make constants public at the knative/pkg repo to avoid hardcoding values. Will fix in another PR.
| metrics.request-metrics-reporting-period-seconds: "10" | ||
|
|
||
| # metrics.opencensus-address specifies the metrics collector address. Metrics are shiped using the opencensus format. | ||
| metrics.opencensus-address: "otel-collector.default.svc:55678" |
There was a problem hiding this comment.
This leaks the implementation detail (opencensus). It is better to have it as a reminder to change the name later on than hiding this option from the end user.
There was a problem hiding this comment.
Will add it in another PR, removing it for now.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #14019 +/- ##
=======================================
Coverage 86.21% 86.21%
=======================================
Files 199 199
Lines 14759 14763 +4
=======================================
+ Hits 12724 12728 +4
- Misses 1733 1734 +1
+ Partials 302 301 -1
☔ View full report in Codecov by Sentry. |
| @@ -144,6 +144,9 @@ var ( | |||
| }, { | |||
| Name: "SERVING_REQUEST_METRICS_BACKEND", | |||
| Value: "", | |||
| }, { | |||
| Name: "SERVING_REQUEST_METRICS_REPORTING_PERIOD_SECONDS", | |||
| Value: "0", | |||
There was a problem hiding this comment.
shouldn't this be a default value?
There was a problem hiding this comment.
Zero or negative value is translated by Opencensus to the default value which is 10s (imho it is implicit, implementation specific and it is better to have an explicit value per case). Here we have the default of 5sec for Prometheus and 60sec for Opencensus (this is taken from knative/pkg constants). They can be changed in the future if we feel that some other values work better in practice.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, skonto The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* fix reporting period * fixes
Fixes #12069
Proposed Changes
metrics.request-metrics-reporting-period-secondsin the observability cm.Release Note