-
Notifications
You must be signed in to change notification settings - Fork 518
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
ClusterLoader - Adding api reposiveness prometheus to tests #493
ClusterLoader - Adding api reposiveness prometheus to tests #493
Conversation
krzysied
commented
Apr 11, 2019
- Making api responsiveness measurement non-failable.
- Addinf api responsiveness measurement to density and load test.
/assign @wojtek-t |
@@ -67,6 +67,12 @@ type apiResponsivenessMeasurementPrometheus struct { | |||
func (a *apiResponsivenessMeasurementPrometheus) Execute(config *measurement.MeasurementConfig) ([]measurement.Summary, error) { | |||
var summaries []measurement.Summary | |||
|
|||
if config.PrometheusFramework == nil { | |||
klog.Errorf("%s: prometheus framework is not provided!") | |||
// TODO(krzysied): for the testing purpose metric is not returning error. |
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.
Instead of TODO(krzysied), please open an issue and do TODO(#issue)
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.
return metrics, errors.NewMetricViolationError("top latency metric", fmt.Sprintf("there should be no high-latency requests, but: %v", badMetrics)) | ||
} | ||
return metrics, nil | ||
// TODO(krzysied): For testing purpose this metric will never return metric violation error. |
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.
Same here
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.
d4b6732
to
0d07406
Compare
clusterloader2/pkg/measurement/common/slos/api_responsiveness_prometheus.go
Show resolved
Hide resolved
// TODO(#498): For testing purpose this metric will never return metric violation error. | ||
// The code below should be | ||
// if len(badMetrics) > 0 { | ||
// return metrics, errors.NewMetricViolationError("top latency metric", fmt.Sprintf("there should be no high-latency requests, but: %v", badMetrics)) |
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.
apiResponsivenessPrometheus{metric}
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.
- Identifier: APIResponsivenessPrometheus | ||
Method: APIResponsivenessPrometheus | ||
Params: | ||
action: start |
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're getting to the point when it makes sense to introduce the "slo bundle" metric. Soon we will have network-programming latency SLI in this categoy, and more is comming - I don't want to grows those configs that much.
Probably a separate PR though.
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.
I will create separate PR.
0d07406
to
a1ae777
Compare
} | ||
|
||
// SummaryTime returns time when summary was created. | ||
func (a *apiResponsivenessPrometheus) SummaryTime() time.Time { |
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.
For consistency with all other metrics - can you move it above PrintSummary?
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.
a1ae777
to
a6cae43
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: krzysied, wojtek-t 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 |