Skip to content

Unify rollup config parsing logic #431

@cwbriones

Description

@cwbriones

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
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions