Allow toggling of latency/count metrics for metrics-generator

[AlexDCraig]

What this PR does:

Allows user to toggle off latency and count metrics in the metrics-generator. It does this by introducing the concept of a subprocessor. The two current subprocessors are span-metrics-count and span-metrics-latency. These subprocessors are toggled on by default, i.e. when "span-metrics" is indicated in the overrides. A user can choose the span-metrics-count subprocessor by indicating it in the overrides, likewise with the -latency one. If a subprocessor that doesn't exist is indicated, it won't be parsed by subprocessor logic but instead will be flagged as a bad processor. Under the hood indicating the span-metrics processor alone enables both subprocessors.

Which issue(s) this PR fixes:
Fixes #2013

Checklist

• [ x ] Tests updated
• Documentation added
• [ x ] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Happy to update documentation if this particular approach for this problem is accepted by maintainers

[mapno]

I like the approach. Left a few comments.

[AlexDCraig]

@zalegrala @mapno ready for another look, I've switched to using a stable key space for subprocessors with the iota pattern and added support for the size metric

[mapno]

Do you think this PR could be simplified if it used booleans (eg. export_count, export_latency and export_size) instead of the subprocessors approach? The current approach leaks much of the logic to instance.updateProcessors, and I think it could be mostly kept to the processor instead.

[AlexDCraig]

Do you think this PR could be simplified if it used booleans (eg. export_count, export_latency and export_size) instead of the subprocessors approach? The current approach leaks much of the logic to instance.updateProcessors, and I think it could be mostly kept to the processor instead.

I like it the way it is but I can change it to whatever your team wants as I would like the feature.

[AlexDCraig]

@mapno FYI in order to rectify your preferred approach with the strategy laid out by Joe in the issue itself: If the idea is to indicate processor in the metrics_generator_processor (e.g. span-metrics-latency, span-metrics-count listed in the overrides) we will no matter what have to have some logic in instance.go to determine which processors are specified so that we can boil them down into one span-metrics processor that contains the right export booleans. Since each processor in that overrides field is added separately by addProcessor there is still complexity needed in instance.go. I don't think this can be done mostly in spanmetrics.go without doing something in the overrides code or adding an overrides field specific to these subprocessors

[mapno]

Apologies, this fell through the cracks and I didn't see a notification until #2013 (comment). I'm sorry.

@mapno FYI in order to rectify your preferred approach with the strategy laid out by Joe in the issue itself: If the idea is to indicate processor in the metrics_generator_processor (e.g. span-metrics-latency, span-metrics-count listed in the overrides) we will no matter what have to have some logic in instance.go to determine which processors are specified so that we can boil them down into one span-metrics processor that contains the right export booleans. Since each processor in that overrides field is added separately by addProcessor there is still complexity needed in instance.go. I don't think this can be done mostly in spanmetrics.go without doing something in the overrides code or adding an overrides field specific to these subprocessors

Don't you think we can move much of the logic in https://github.com/grafana/tempo/pull/2070/files#diff-2b2bf6091e8c6446ffe339e21fd2f917d4ba0c172a76a226153f53121181b202R127-R171 to the processor? The instance can just pass all the processors named span-metrics.*, and the processor handles which processors it wants running.

Anyway, I don't block this much longer after not replaying so long.

[AlexDCraig]

@mapno I've rebased

[AlexDCraig]

@mapno Sorry, is that last comment supposed to mean an approval is imminent? That's why I said I rebased 😆

[mapno]

Sorry, I wasn't clear. I was hoping we could move much of the logic in https://github.com/grafana/tempo/pull/2070/files#diff-2b2bf6091e8c6446ffe339e21fd2f917d4ba0c172a76a226153f53121181b202R127-R171 to the processor. Spanmetrics can take the processors that are enabled and work out which subprocessors need to be running.

Nonetheless, I don't want to block this further if you're not convinced that approach is any better. Let me know and we can see to approving and merging.

[AlexDCraig]

@mapno As I said above, you will still need logic in instance.go so I am not convinced that approach is better. The reason why is these "subprocessors" are considered normal processors (as in, they are received from the overrides list just like processors), so "span-metrics-latency", "span-metrics-count" are received in instance.go and you can't create a separate processor for both, so instance.go will have to parse out which subprocessors are there in order to get one processor with the right configs out of it. So going the other way, the logic is not totally contained in the processor, so we should just keep things this way

[mapno]

I was away for a few days, I'm ok to merge. Can you solve the conflict? Thanks

[AlexDCraig]

@mapno Rebased, thanks

[mapno]

Merging, thanks for the patience.