Basic functional options support to metrics assertion in integration tests #2522
Conversation
|
This feels bit complicated, but if we don't find a simpler way, I'm fine with it. Each time I need to write new metric-based condition, I need to find out how. As an alternative, what do you think about a possibility to use PromQL expressions for checking these conditions? |
There was a problem hiding this comment.
Looks good, one suggestion though (:
It's nicely extensible.
@pstibrany while PromQL would be nice it will be massive pain to implement, plus we have only current metric values and PromQL works on step aligned query etc, plus we will need to block 98.9 % of functions operators, etc so literally writing our own parser so it's just complex ;p
I think I like Marco's approach, just we could simplify a bit.
integration/e2e/composite_service.go
Outdated
There was a problem hiding this comment.
I get the use case, plus, sometimes you really want only certain label, so we could have WithLabelEquals(name, value string)
However what do you think about simplifying interfaces and having just single metric name? We can save on []string{ boilerplate 🤔
There was a problem hiding this comment.
However what do you think about simplifying interfaces and having just single metric name? We can save on []string{ boilerplate
We could, but then the testers *AmongTwo wouldn't work anymore. Cortex is not using it, but Thanos does. Thoughts?
There was a problem hiding this comment.
so we could have WithLabelEquals(name, value string)
Agree, done.
|
This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions. |
|
We decided to move forward with this, but haven't got the time to do it yet. |
…tests Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
more-flexible-wait-metrics
branch
from
9bb8c87 to
69630eb
Compare
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
|
|
||
| time.Sleep(2 * time.Second) | ||
| require.NoError(t, am.WaitSumMetrics(e2e.Equals(0), "cortex_alertmanager_config_invalid")) | ||
| require.NoError(t, am.WaitSumMetricsWithOptions(e2e.Equals(0), []string{"cortex_alertmanager_config_invalid"}, |
Signed-off-by: Marco Pracucci <[email protected]>
…tests (cortexproject/cortex#2522) * Basic functional options support to metrics assertion in integration tests Signed-off-by: Marco Pracucci <[email protected]> * Added WithLabelMatchers() option to integration tests Signed-off-by: Marco Pracucci <[email protected]> * Removed time.Sleep() from TestAlertmanagerStoreAPI Signed-off-by: Marco Pracucci <[email protected]> * Removed last time.Sleep() from integration tests Signed-off-by: Marco Pracucci <[email protected]> * Renamed WaitMissingMetric() into WaitRemovedMetric() Signed-off-by: Marco Pracucci <[email protected]>
5 participants
What this PR does:
In #2515 (by Joe) I wanted to assert on
cortex_request_duration_secondsin the integration tests, but I wasn't able because unable to assert on a histogram's count value.In this PR I'm doing baby steps do add functional options support to metrics assection in integration tests. Could be improved in many ways (and I will do in subsequent PRs if this one is approved), but in this PR I would prefer to keep the change set as smallest as possible to validate the basic design.
/cc @bwplotka
Which issue(s) this PR fixes:
Fixes #2975.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]