[issue-#6232] Added support for metrics signal in groupbyattrsprocessor

[bertysentry]

Description:
Added support for Metrics in groupbyattrsprocessor

Link to tracking Issue:
Issue #6232

Testing:
All unit tests have been updated to test metrics

Documentation:
README.md has been updated to mention support for metrics.

[MovieStoreGuy]

Just two things but nothing that would impact runtime.

[bertysentry]

Rebased on main to make sure things will build smoothly. @MovieStoreGuy Could you please trigger the GitHub actions?

[bertysentry]

@owais @anuraaga If you guys have some time to review this new feature, that would be awesome. It's already been tested to transform flat metrics from Prometheus to OTLP to DataDog (which leverages the concept of Resources to create corresponding hosts), and it works great in real use cases.

[bertysentry]

@anuraaga As code owner, please review, thank you in advance!

[anuraaga]

I am somewhat worried about corner cases for metrics - for example is it ok to combine points from different collection intervals into the same Metric? I guess it is but I'm not sure if it could have some implications.

The code otherwise seems fine though, but looking at the linked issue, the motivation seems to be a problem with the Prometheus receiver populating resource information like host.name in metric instead of resource. Shouldn't the Prometheus receiver be fixed to fill in resource properly instead so users don't need a processor to fix it?

[bertysentry]

@anuraaga I do not understand what scenario you imply when you say:

for example is it ok to combine points from different collection intervals into the same Metric?

DataPoints are not "combined" here. DataPoints are simply copied from the original Resource to the newly created Resource with matching Attributes. So the origin and time of the DataPoint really shouldn't matter as the entire point is copied, but I may miss something (as I'm very new to OpenTelemetry, so please forgive anything stupid I may say 😅)

Regarding the Prometheus Receiver, it is true that it could have configurable options to create Resources as per some specified Attributes keys found on the imported metrics, but that may be out of the scope of a receiver, especially since we have the groupbyattrs processor.

And it doesn't seem logical to me that groupbyattrs would work on Traces and Logs, but not on Metrics. I still think this is a nice addition, given the flat nature of Prometheus metrics notably (but not only!).

Thank you!

[anuraaga]

The scenario I think of, which I am not entirely sure is correct since I may be misreading, is that there are some data points in one Metric, and some in another Metric, that both have the same attributes. From what I could tell they will get merged into the same Metric. I don't know if this is the correct semantics for the Metric class.

Regarding the Prometheus Receiver, it is true that it could have configurable options to create Resources as per some specified Attributes keys found on the imported metrics

I don't know if it even needs to be configurable, isn't it more of a Prometheus data model mapping issue? I believe the spec is currently working on more robust mapping between the two this could be an issue to raise there. It may turn out that the receiver is exactly where this should be mapped in an official way.

And it doesn't seem logical to me that groupbyattrs would work on Traces and Logs, but not on Metrics

I think it's because while Prometheus is flat, OTLP, which is the collector data model, isn't so the logic is not as clear (as above point). It took a lot of reading of the code to reason through the behavior and confirm that different data types wouldn't be merged into one. It's much more complex than spans and logs.

That doesn't mean there shouldn't be one but I don't think it's a trivial transformation. We need a thorough review from a metrics expert I think, maybe @bogdandrutu

[bertysentry]

The scenario [...] is that there are some data points in one Metric, and some in another Metric, that both have the same attributes. From what I could tell they will get merged into the same Metric.

To be "merged" into the same Metric instance, the DataPoints must have the same specified Attributes and also belong to a Metric with the same Name and same DataType. In practice, you should never have 2 separate Metric instances with the exact same name, unit and datatype (even though it doesn't make any difference, as they will be exported in the very same way).

Say you have the below metric points as input:

• Resource: {host="my-exporter"}
  • Metric: my_metric_total (Sum)
    • DataPoint: {host="host-A",id="eth0"} 1234
    • DataPoint: {host="host-A",id="eth0"} 1235
    • DataPoint: {host="host-B",id="eth0"} 0
    • DataPoint: {host="host-B",id="eth0"} 1
  • Metric: other_metric (Gauge)
    • DataPoint: {host="host-A",id="eth0"} 13
    • DataPoint: {host="host-A",id="eth0"} 2
    • DataPoint: {host="host-B",id="eth0"} 7
    • DataPoint: {host="host-B",id="eth0"} 1

And you configure groupbyattrs to group by the host Attribute. You will get the below metric points in the output:

• Resource: {host="host-A"}
  • Metric: my_metric_total (Sum)
    • DataPoint: {id="eth0"} 1234
    • DataPoint: {id="eth0"} 1235
  • Metric: other_metric (Gauge)
    • DataPoint: {id="eth0"} 13
    • DataPoint: {id="eth0"} 2
• Resource: {host="host-B"}
  • Metric: my_metric_total (Sum)
    • DataPoint: {id="eth0"} 0
    • DataPoint: {id="eth0"} 1
  • Metric: other_metric (Gauge)
    • DataPoint: {id="eth0"} 7
    • DataPoint: {id="eth0"} 1

BTW, the name of this processor is a bit misleading, because it's actually splitting Metrics into several Resources, rather than grouping them.

Regarding the Prometheus Receiver, it is true that it could have configurable options to create Resources as per some specified Attributes keys found on the imported metrics

I don't know if it even needs to be configurable, isn't it more of a Prometheus data model mapping issue? I believe the spec is currently working on more robust mapping between the two this could be an issue to raise there. It may turn out that the receiver is exactly where this should be mapped in an official way.

There is no such concept in Prometheus' data model, like Resources or Hosts, unfortunately. Prometheus is very flat, as you said, so the OpenTelemetry receiver for Prometheus will not be able to map certain Prometheus labels into OpenTelemetry Resources, I'm afraid.

That doesn't mean there shouldn't be one but I don't think it's a trivial transformation. We need a thorough review from a metrics expert I think, maybe @bogdandrutu

I totally agree: I started working on this thinking "gonna be a piece of cake, just copy-paste the Log part and replaceAll("Log", "Metric"), but it was more complicated with the different data types and the DataPoints, which hold the Attributes (and not the Metrics).

@bogdandrutu Your input is welcome, thank you!

[anuraaga]

To be "merged" into the same Metric instance, the DataPoints must have the same specified Attributes and also belong to a Metric with the same Name and same DataType.

Yeah - I just don't know if OTLP indicates that there can't be more than one Metric with the same name in a batch, and if so whether it's semantically equivalent to merge them. I suspect so, just don't think it's clear.

There is no such concept in Prometheus' data model, like Resources or Hosts, unfortunately.

Yup, but I guess that there could still be a defacto mapping with pretty good coverage. For example, if host.name, hostname, host-name, host were all mapped to OTel host.name resource attribute, I suspect 99.9% of users would be covered, without having to add this processor. That would be a better UX in general. This is a hypothesis of course but if it seems conventional enough within Prometheus than I would definitely recommend it over having users to explicitly define it. It may not be conventional enough as my 99.9% number is made up. But it's my gut feeling.

And again that doesn't mean there shouldn't be a group by attr processor, but it needs to be clear of corner cases within OTLP's not-flat model, and I think its presence could be considered independent of a better mapping within the receiver.

[bertysentry]

@anuraaga @MovieStoreGuy Please help!

The build is failing with this error:

Generated code is out of date, please run "make generate" and commit the changes in this PR.
Error: Process completed with exit code 1.

However, when I run make generate as advised in the error message, there is no change in the source, hence nothing that I can commit.

Any idea?

(Apart from that: I rebased the PR, squashed latest changes, fixed a bug, added unit tests as required and updated README.md, please review! 😉)

[bertysentry]

@bogdandrutu @anuraaga @Aneurysm9 All tests are now successful. *groupbyattrs processor now fully supports Metrics and documentation has been updated accordingly to clarify how the processor works.
Please review and approve if you guys are okay with the new feature.
Thank you in advance for your time!

[codeboten]

Pinging @pmm-sumo as well as the codeowner for the component

[pmm-sumo]

This is a very useful extension of the processor

[bertysentry]

As everybody approved (@pmm-sumo @Aneurysm9 @MovieStoreGuy) and the build is all good, could someone please push the Merge button? @anuraaga ? Thank you!

[MovieStoreGuy]

@bogdandrutu, @jpkrohling, or @tigrannajaryan ?

[jpkrohling]

I added this to my queue. I trust the folks who reviewed this already, I'll just take a quick look before hitting the button.

[jpkrohling]

Looks good, just a couple of typos and minor comments.

[bertysentry]

Looks good, just a couple of typos and minor comments.

All reported typos and suggestions have been implemented. Thank you for the review!

[bertysentry]

@jpkrohling All checks have passed now, so we're good to go. Thank you!

[bertysentry]

Yay! 😊