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.

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

Unify rollup config parsing logic #431

Open
cwbriones opened this issue Jun 30, 2020 · 0 comments
Open

Unify rollup config parsing logic #431

cwbriones opened this issue Jun 30, 2020 · 0 comments

Comments

@cwbriones
Copy link
Contributor

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 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")
private Predicate<String> provideRollupPopulationPredicate(final Config config) {
    // copy-pasta code from MetricsDiscovery.java
}

// in MetricsQueryConfigImpl.java, same for MetricsDiscovery.java
public MetricsQueryConfig(
    @Named("shouldPopulateRollups") Predicate<String> shouldPopulateRollups
) {
    // .. save and use like you're doing in this PR
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant