Make publishing average configurable

[jonatan-ivanov]

See #6843, in 1.x we can keep publish average but we can let the users disable it with a config option.

[jaxxnitt]

I would like to work on this.

Just to confirm the expected behavior for 1.x:

keep the current default (publish averages),

add a new config option in DatadogConfig to enable/disable publishing averages,

and use that flag in DatadogMeterRegistry to control whether .avg is emitted.

Then the default can be flipped in 2.x as discussed in #6843.
Is this the intended direction?

[jonatan-ivanov]

@jaxxnitt Thank you for the offer, your description is correct, please feel free to open a PR.

[jaxxnitt]

I've opened a PR implementing this: #6903

It adds the publishAverage config option with the current default (enabled), and updates Timer/FunctionTimer/Summary emission accordingly. Happy to adjust anything if needed.

[jonatan-ivanov]

Thank you!
In the meantime @shakuzen noticed that I missed one detail in your description: this should work for all of the registries that publish avg not only Datadog. Would you mind updating the PR?

[jaxxnitt]

Thanks for the review, @jonatan-ivanov!

Before I update the PR:
You mentioned this should work for all registries that publish .avg. Since only push-based registries emit average metrics, and those share PushRegistryConfig as the common config interface (whether or not they pass through StepRegistryConfig), should the new publishAverage() flag be added to PushRegistryConfig?
Just want to confirm the correct interface before I refactor the PR.

[shakuzen]

That's a good question @jaxxnitt. I haven't given a lot of thought to this yet, but I think it could make sense for the option to be on DistributionStatisticsConfig similar to how that is used to control whether client-side percentiles or a histogram is tracked (and published). It could also make sense on the registry config (e.g. PushRegistryConfig) as you suggested. One trade-off is that if it is on the registry config, it can only be controlled at the registry level, whereas if it is on the DistributionStatisticsConfig, it can be controlled per meter. I imagine most users want it on or off for the whole registry, but it's also logical and in line with other options on DistributionStatisticsConfig for it to be there. I would lean toward putting it there, but as I said, I haven't spent enough time thinking about all the implications, so if you have some thoughts on which approach is better and why, please share.

[jaxxnitt]

Thanks, @shakuzen , that explanation helps a lot. I don't have deep context on this yet, but based on what you described, adding the option to DistributionStatisticsConfig does seem more appropriate since it's consistent with how other per-meter publication settings are controlled. I'll explore that direction and update the PR tomorrow.

[jonatan-ivanov]

I think I would favor DistributionStatisticsConfig as well. If a registry would be a preferred option, not sure I would ad to PushRegistryConfig but instead to every registry config where we already publish avg. Adding to PushRegistryConfig might be confusing for registries that do not push avg and a user sets it to true for a certain registry. DistributionStatisticsConfig is more decoupled, hope it does not have the same issue.

[jaxxnitt]

Hi @shakuzen @jonatan-ivanov,

While implementing the “publish average” flag, I reviewed registries that currently emits average values — Datadog, CloudWatch, Humio, SignalFx, Wavefront, Dynatrace exporter v1.

Across all of these, the code paths that emit AVG metrics do not receive a DistributionStatisticConfig instance. Other distribution-related settings (like isPublishingPercentiles() and isPublishingHistogram()) work because the registries that use those features receive the DistributionStatisticConfig in the corresponding meter wrapper constructors. The registries that emit averages do not receive it anywhere.

Since the config is never passed into those registry adapters, the “publish average” flag isn’t accessible at the point where the AVG metrics are emitted.

[jonatan-ivanov]

It does but it might be a bit hidden:

• DatadogMeterRegistry: It inherits newXyz methods (e.g.: newTimer) from StepMeterRegistry which receive DistributionStatisticConfig so it is possible to pass the information if the user wants avg or not.
• CloudWatchMeterRegistry: Same as DatadogMeterRegistry.
• HumioMeterRegistry: Same as DatadogMeterRegistry.
• SignalFxMeterRegistry: Deprecated, don't need to be implemented.
• WavefrontMeterRegistry: Deprecated, don't need to be implemented.
• DynatraceMeterRegistry: This is a bit more tricky because it overrides the newXyz methods and returns different implementations based on config (can be same as DatadogMeterRegistry depending if this is implemented in StepTimer or AbstractTimer) but also, the Dynatrace protocol might enforce us to send the avg, if that's the case, this should not be implemented, let me ask around.