[metrics] Dependency resolution metrics #1657
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
do-not-merge/work-in-progress
benluddy
reviewed
Jul 20, 2020
anik120
force-pushed
the
dependency-resolution-metrics
branch
2 times, most recently
from
83de9b0 to
f075a5d
Compare
anik120
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
This PR failed tests for 1 times with 0 individual failed tests and 119 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
f075a5d to
d87bd37
Compare
|
This PR failed tests for 1 times with 0 individual failed tests and 119 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
d87bd37 to
8952043
Compare
|
This PR failed tests for 1 times with 1 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
awgreene
reviewed
Jul 23, 2020
benluddy
reviewed
Jul 23, 2020
Comment on lines
131
to
133
| res := resolver.NewOperatorsV1alpha1Resolver(lister, crClient, opClient.KubernetesInterface()) | ||
| success := metrics.RegisterDependencyResolutionSuccess | ||
| failure := metrics.RegisterDependencyResolutionFailure |
There was a problem hiding this comment.
My preference would be to inline these on L144 so that readers don't need to jump around to find out what these parameters are, but that's a nit and I can agree to disagree.
There was a problem hiding this comment.
I think Ben's suggestion would make this clearer.
There was a problem hiding this comment.
@benluddy @awgreene the reason I chose not to go inline was coz then that line reads as
resolver: resolver.NewInstrumentedResolver(resolver: resolver.NewInstrumentedResolver(operatorV1alpha1Resolver, successMetricsEmitter, failureMetricsEmitter),, metrics.RegisterDependencyResolutionSuccess, metrics.RegisterDependencyResolutionFailure),
which goes way beyond the 120 character vertical ruler I keep for myself.
I am changing the variable names to operatorsV1alpha1Resolver successMetricsEmitter and failureMetricsEmitter to have the reader hopefully not jump around too much. Let me know if that looks good once I've pushed those changes.
pkg/metrics/metrics.go
Outdated
| @@ -269,3 +286,11 @@ func UpdateSubsSyncCounterStorage(sub *olmv1alpha1.Subscription) { | |||
| counterValues.channel = sub.Spec.Channel | |||
| } | |||
| } | |||
|
|
|||
| func RegisterDependencyResolutionSuccess(time *float64) { | |||
There was a problem hiding this comment.
Why is the argument a pointer? Can it be a time.Duration value instead? Converting to float64 and throwing away type information at the measurement site seems premature.
There was a problem hiding this comment.
@benluddy I had this argument as just float64 to begin with, but switched over to a pointer when I realized implementing a fake metricEmitter func will be easier if I can modify a value in place.
Converting to float64 and throwing away type information at the measurement site seems premature.
My argument against that is that for this particular function, RegisterDependencyResolutionSuccess that registers a value for a summary, we will only ever need a float64 coz the function Observe only takes a float64
There was a problem hiding this comment.
The contract of the function is to record a metric corresponding to the duration of a dependency resolution success. Why does the caller of this function need to adapt the input to the Prometheus client? If the underlying metrics reporter were to be swapped out with a client that expects a different value type, this function's signature should not need to change.
pkg/metrics/metrics.go
Outdated
| @@ -170,6 +170,21 @@ var ( | |||
| []string{NAMESPACE_LABEL, NAME_LABEL, VERSION_LABEL, PHASE_LABEL, REASON_LABEL}, | |||
| ) | |||
|
|
|||
| dependencyResolutionSucceeded = prometheus.NewSummary( | |||
| prometheus.SummaryOpts{ | |||
| Name: "dependency_resolution_succeeded", | |||
There was a problem hiding this comment.
It would be good to at least include the unit suffix in the name (https://prometheus.io/docs/practices/naming/#metric-names).
There was a problem hiding this comment.
We may also want to consider that these metrics will be available on telemeter - it's somewhat difficult to know if this metric is an "OLM Metric" based on this name. We had questions on slack regarding if OLM has any operator metrics because the existing metrics didn't make it clear.
There was a problem hiding this comment.
If we were to incorporate both points, olm_dependency_metrics_succeeded_summary is the name coming to my mind. Isn't that a tad bit long?
There was a problem hiding this comment.
I would pick something like olm_resolution_duration_seconds.
pkg/metrics/metrics.go
Outdated
| @@ -194,6 +209,8 @@ func RegisterCatalog() { | |||
| prometheus.MustRegister(subscriptionCount) | |||
| prometheus.MustRegister(catalogSourceCount) | |||
| prometheus.MustRegister(SubscriptionSyncCount) | |||
| prometheus.MustRegister(dependencyResolutionSucceeded) | |||
There was a problem hiding this comment.
Can success/failure be two values for a single label instead of having two completely separate metrics?
There was a problem hiding this comment.
@benluddy if we want two separate percentile brackets for the two scenarios, I think have two separate metrics is cleaner.
At the server side we can register values into individual metrics cleanly, and at the client side (whatever visualization tool we use) we can separately query for the metrics without jumping too many hoops.
There was a problem hiding this comment.
Each combination of label values gets its own time series. They're designed to be queried this way.
There was a problem hiding this comment.
@benluddy changed it to SummaryVec that allows labels.
|
This PR failed tests for 1 times with 1 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
8952043 to
cc35135
Compare
|
This PR failed tests for 1 times with 2 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
|
/test e2e-aws-console-olm |
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
cc35135 to
f8fda06
Compare
benluddy
reviewed
Jul 24, 2020
|
This PR failed tests for 1 times with 2 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
f8fda06 to
47e52d4
Compare
openshift-ci-robot
added
the
needs-rebase
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
47e52d4 to
9b025fa
Compare
openshift-ci-robot
removed
the
needs-rebase
|
@benluddy updated the PR, PTAL. Thanks! |
|
/test e2e-gcp-upgrade |
awgreene
approved these changes
Jul 27, 2020
| } | ||
|
|
||
| func TestInstrumentedResolver(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
nit: Please remove the newline
benluddy
reviewed
Jul 27, 2020
This PR introduces two histogram metrics to help in analysing the time taken by dependency resolution requests.
anik120
force-pushed
the
dependency-resolution-metrics
branch
from
9b025fa to
1db2017
Compare
|
/approve 👍 |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anik120, awgreene, benluddy 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Tagging @openshift/openshift-team-monitoring to review the new metric we plan to expose. If you have any complains, please let us know and we will update the metric. |
|
@smarterclayton OLM would like to add this metric to Telemeter, can you please review? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change:
This PR introduces two histrogram metrics to help in analysing the
time taken by dependency resolution requests.
Motivation for the change:
Reviewer Checklist
/docs