Adding a dynamic dispatch to Aggregator Selector

[dawid-nowak]

@TommyCpp
Have a look and let me know if something like this could work?

Rationale:
As a user, I would like to be able to supply my own aggregator selector so I can better control how data is aggregated.
If no aggregator is set by a user, sensible defaults are provided.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Dawid Nowak (f23311c)

[djc]

Style nits: this should be named NopAggregator (maybe NoopAggregator is even easier to understand for most readers?) and you should just leave off the ().

[TommyCpp]

Yeah, I think we most used Noop[Component Name] to name the no-op components.

[dawid-nowak]

i think i will keep this example as is and add another example with a custom aggregator selector.

[dawid-nowak]

removed altogether

[TommyCpp]

Maybe move this part to the top to keep the imports at the same place?

[dawid-nowak]

moved to a different example and moved to the top

[dawid-nowak]

• Added a separate example for custom aggregator selector
• By the way I had to change const LEMONS_KEY: Key = Key::from_static_str("ex.com/lemons"); to
  const LEMONS_KEY: Key = Key::from_static_str("lemons"); as otherwise otlp collector is complaining about it when converting from otlp to prometheus

2021-03-29T13:48:32.013Z	error	prometheusexporter/collector.go:259	failed to convert metric ex.com.one: "ex.com/lemons" is not a valid label name for metric "perf_ex_com_one"	{"component_kind": "exporter", "component_type": "prometheus", "component_name": "prometheus"}

[TommyCpp]

LGTM:+1: Thanks

[jtescher]

Are all these other box changes still needed?

[dawid-nowak]

I think so. I tried it before and was running into problems. If my understanding is correct if you remove Box::dyn then the compiler does static dispatch and turns T into a concrete type. So without dyns, it works in a situation where both opentelemetry_otlp::new_metrics_pipeline(tokio::spawn, delayed_interval) and .with_aggregator_selector() use the same type T.

[jtescher]

If you replace the method body with the full:

pub fn with_aggregator_selector<T>(
    self,
    aggregator_selector: T,
) -> OtlpMetricPipelineBuilder<T, ES, SP, SO, I, IO>
where
    T: AggregatorSelector + Send + Sync + 'static,
{
    OtlpMetricPipelineBuilder {
        aggregator_selector,
        export_selector: self.export_selector,
        spawn: self.spawn,
        interval: self.interval,
        export_config: self.export_config,
        tonic_config: self.tonic_config,
        resource: self.resource,
        stateful: self.stateful,
        period: self.period,
        timeout: self.timeout,
    }
}

Then you don't need any other change.

[dawid-nowak]

I have a feeling I am missing something here.
If I use the snippet from above I am getting compilation errors on both the main and this branch.
If I remove Box::dyns I am pretty much back to the code that is currently on the main branch. In there the aggregator selector in new_metrics_pipeline must be of the same type as in with_aggreagtor_selector.

[dawid-nowak]

i have removed more dyns from new_metrics_pipeline_with_selector but i am not sure if i can remove the remaining ones

[jtescher]

Something along the lines of 88b1904 should work

[dawid-nowak]

@jtescher I have missed that. The code is much simpler indeed, thanks. I have merged your suggestions, so hopefully, we are good now.

[jtescher]

Looks good, thanks @dawid-nowak