Create ValidateMetrics #5988
Create ValidateMetrics #5988
Conversation
The validation package is exporting Prometheus counters that are using the default Prometheus registry. This is problematic when unit tests are running tests in parallel because the distributors and ingesters will use the exported counters in their tests. Signed-off-by: Charlie Le <[email protected]>
| count := 100000 | ||
| target := time.Second | ||
|
|
||
| // find right count to make sure that push takes given target duration. | ||
| for { | ||
| req := generateSamplesForLabel(labels.FromStrings(labels.MetricName, fmt.Sprintf("test-%d", count)), count) | ||
|
|
||
| start := time.Now() | ||
| _, err := i.Push(ctx, req) | ||
| require.NoError(t, err) | ||
|
|
||
| elapsed := time.Since(start) | ||
| t.Log(count, elapsed) | ||
| if elapsed > time.Second { | ||
| break | ||
| } | ||
|
|
||
| count = int(float64(count) * float64(target/elapsed) * 1.5) // Adjust number of samples to hit our target push duration. | ||
| } | ||
|
|
||
| // Now repeat push with number of samples calibrated to our target. |
There was a problem hiding this comment.
is this change related to this PR?
There was a problem hiding this comment.
It's not, I can move it into a separate PR if you'd like. The change was included to address an issue with the test's flakiness. I added a note about it in the description of the PR:
The TestIngester_inflightPushRequests test was updated as well since it could fail if there were too many samples pushed (3600000 (also number of seconds in an hour)) and an out of bounds error would be thrown.
Signed-off-by: Charlie Le <[email protected]>
| @@ -1,32 +1,34 @@ | |||
| package ingester | |||
|
|
|||
| import ( | |||
| "github.com/cortexproject/cortex/pkg/util/validation" | |||
There was a problem hiding this comment.
Use formatting so the lint passes
These metrics could be registered by the distributor and the ingester in single binary mode. Signed-off-by: Charlie Le <[email protected]>
Signed-off-by: Charlie Le <[email protected]>
| ) | ||
| func registerCollector(r prometheus.Registerer, c prometheus.Collector) { | ||
| err := r.Register(c) | ||
| if err != nil && !errors.As(err, &prometheus.AlreadyRegisteredError{}) { |
There was a problem hiding this comment.
Do we expect to have prometheus.AlreadyRegisteredError error?
There was a problem hiding this comment.
Yes, it will happen when the distributor and ingester are both instantiated in the test TestCortex. Also, these metrics could be registered by the distributor and the ingester in single binary mode.
| []string{discardReasonLabel, "user"}, | ||
| ) | ||
| registerCollector(r, discardedExemplars) | ||
| discardedMetadata := prometheus.NewCounterVec( |
There was a problem hiding this comment.
I wonder if we can just use promauto package here which automatically registers the collector.
There was a problem hiding this comment.
We cannot because promauto does MustRegister which will panic if there is a registration error (this includes AlreadyRegisteredError).
The validation package is exporting Prometheus counters that are using the default Prometheus registry. This is problematic when unit tests are running tests in parallel because the distributors and ingesters will use the exported counters in their tests.
What this PR does:
Creates a ValidateMetrics struct to house the metrics defined in the validation package so that it can be registered to a registry.
The
TestIngester_inflightPushRequeststest was updated as well since it could fail if there were too many samples pushed (3600000 (also number of seconds in an hour)) and an out of bounds error would be thrown.Which issue(s) this PR fixes:
Fixes: #5659
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]