-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 metrics reporting period #14019
Fix metrics reporting period #14019
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be a default value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}, { | ||
Name: "TRACING_CONFIG_BACKEND", | ||
Value: "", | ||
|
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.
We need to make constants public at the knative/pkg repo to avoid hardcoding values. Will fix in another PR.