Skip to content
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.

Sign up for GitHub

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

adjust bld prometheus ext test for concurrent tests, cross namespace … #17635

Merged
merged 1 commit into from
Dec 7, 2017
Merged

adjust bld prometheus ext test for concurrent tests, cross namespace … #17635

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions test/extended/prometheus/prometheus_builds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ var _ = g.Describe("[Feature:Prometheus][Feature:Builds] Prometheus", func() {
activeTests := map[string][]metricTest{
activeBuildQuery: {
metricTest{
labels: map[string]string{"name": "frontend-1"},
greaterThan: true,
labels: map[string]string{"name": "frontend-1"},
greaterThanEqual: true,
},
},
}
Expand All @@ -88,14 +88,16 @@ var _ = g.Describe("[Feature:Prometheus][Feature:Builds] Prometheus", func() {
terminalTests := map[string][]metricTest{
buildCountQuery: {
metricTest{
labels: map[string]string{"phase": string(buildapi.BuildPhaseComplete)},
greaterThan: true,
labels: map[string]string{"phase": string(buildapi.BuildPhaseComplete)},
greaterThanEqual: true,
},
metricTest{
labels: map[string]string{"phase": string(buildapi.BuildPhaseCancelled)},
labels: map[string]string{"phase": string(buildapi.BuildPhaseCancelled)},
greaterThanEqual: true,
},
metricTest{
labels: map[string]string{"phase": string(buildapi.BuildPhaseFailed)},
labels: map[string]string{"phase": string(buildapi.BuildPhaseFailed)},
greaterThanEqual: true,
},
},
}
Expand All @@ -120,10 +122,14 @@ type prometheusResponseData struct {
}

type metricTest struct {
labels map[string]string
greaterThan bool
value float64
success bool
labels map[string]string
// we are not more precise (greater than only, or equal only) becauses the extended build tests
// run in parallel on the CI system, and some of the metrics are cross namespace, so we cannot
// reliably filter; we do precise count validation in the unit tests, where "entire cluster" activity
// is more controlled :-)
greaterThanEqual bool
value float64
success bool
}

func runQueries(metricTests map[string][]metricTest) {
Expand Down Expand Up @@ -209,10 +215,10 @@ func labelsWeWant(sample *model.Sample, labels map[string]string) bool {
}

func valueWeWant(sample *model.Sample, tc metricTest) bool {
//NOTE - we could use SampleValue has an Equals func, but since SampleValue has no GreaterThan,
//NOTE - we could use SampleValue has an Equals func, but since SampleValue has no GreaterThanEqual,
// we have to go down the float64 compare anyway
if tc.greaterThan {
return float64(sample.Value) > tc.value
if tc.greaterThanEqual {
return float64(sample.Value) >= tc.value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't the issue that we're getting a non-zero value when we expect zero? how does this help that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

greaterThan was set to false in those cases, so we fell back to the == check below

if you noticed I set greaterThanEqual to true for cancelled / failed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

}
return float64(sample.Value) == tc.value
return float64(sample.Value) < tc.value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems slightly odd that you're changing == to < here, not least given AFAICS there is no metricTest which doesn't have greaterThanEqual set, so this code path isn't actually ever used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you didn't explicitly request greaterThanEqual, then you're implicitly requesting less than... (at least under the new semantics introduced by this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct that the code path is not used currently; I wanted the code in place to facilitate usage of greaterThanEqual not being set in the future

and yes, given the precise values are not guarantee-able when this test is run in parallel / in conjunction with other tests, I wanted to have either "greater than or equal" or "less than or equal".

}