You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
a. If rollups should be populated for a given metric (ref)
b. If rollups should be utilized in the query proxy for a given metric (ref)
These were separated to allow for populating troublesome metrics without actually affecting user queries. For example, a problematic metric could be hitting some edge-case in rollup population, populating the rollups with invalid data. If we know about such a case, we want to enable the rollups to debug them but we don't want the queries to use them until we're sure they work.
Problem
As such, we have two places that use this configuration: MetricsDiscovery and the KairosDbServiceImpl via MetricsQueryConfigImpl. In #428 we changed the proxy layer to use both configuration values, the logic being that we shouldn't try to use rollups we're not currently populating. This resulted in us parsing the configuration in more than one place.
Proposed Fix
I'm just going to quote a comment I left on the original PR:
Ideally I'd like to have the rollup eligibility check shared between the query side and the discovery side to ensure they're the same.
That is, something like
// ... in MainModule.java@Named("shouldPopulateRollups")
privatePredicate<String> provideRollupPopulationPredicate(finalConfigconfig) {
// copy-pasta code from MetricsDiscovery.java
}
// in MetricsQueryConfigImpl.java, same for MetricsDiscovery.javapublicMetricsQueryConfig(
@Named("shouldPopulateRollups") Predicate<String> shouldPopulateRollups
) {
// .. save and use like you're doing in this PR
}
The text was updated successfully, but these errors were encountered:
Background
There's some configuration that determines
a. If rollups should be populated for a given metric (ref)
b. If rollups should be utilized in the query proxy for a given metric (ref)
These were separated to allow for populating troublesome metrics without actually affecting user queries. For example, a problematic metric could be hitting some edge-case in rollup population, populating the rollups with invalid data. If we know about such a case, we want to enable the rollups to debug them but we don't want the queries to use them until we're sure they work.
Problem
As such, we have two places that use this configuration:
MetricsDiscovery
and theKairosDbServiceImpl
viaMetricsQueryConfigImpl
. In #428 we changed the proxy layer to use both configuration values, the logic being that we shouldn't try to use rollups we're not currently populating. This resulted in us parsing the configuration in more than one place.Proposed Fix
I'm just going to quote a comment I left on the original PR:
The text was updated successfully, but these errors were encountered: