[mdatagen] Re-Aggregate Metric by Attributes

[shalper2]

Please note this is still very much a work in progress.

Description

Modified mdatagen to allow for two new fields in the configuration yaml of metrics. These new configuration options would allow for a user to enable or disable attributes (i.e. reduce dimensionality of metrics being generated) from their collector configuration. The modified MetricsBuilder generated by mdatagen automatically re-aggregates metrics based on the enabled set of attributes. Additionally, different aggregation strategies can be specified by the collector administrator.

The changes to the config.yaml are:

receiver:
  someMetricReceiver:
    ...
    metrics:
    ...
    attributes: # new field
      some.attribute.name:
         enabled: < true | false >
      aggregation_strategy: < sum | avg | min | max >

Note that this will not change metadata.yaml so users can expect the same overall mdatagen experience. Default values for the modified MetricsBuilder are to enable all attributes included in mdatagen.

Link to tracking issue

Fixes 10726

Testing

Some testing has been done using the modified mdatagen with existing contrib receivers (i.e. hostmetricsreceiver)

Documentation

[codecov]

Codecov Report

❌ Patch coverage is 58.21288% with 318 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.70%. Comparing base (787ae29) to head (3e728f5).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...leconnector/internal/metadata/generated_metrics.go	51.19%	94 Missing and 8 partials ⚠️
...plereceiver/internal/metadata/generated_metrics.go	51.19%	94 Missing and 8 partials ⚠️
...mplescraper/internal/metadata/generated_metrics.go	51.19%	94 Missing and 8 partials ⚠️
cmd/mdatagen/internal/metadata.go	40.00%	2 Missing and 1 partial ⚠️
...pleconnector/internal/metadata/generated_config.go	92.68%	2 Missing and 1 partial ⚠️
...mplereceiver/internal/metadata/generated_config.go	93.61%	2 Missing and 1 partial ⚠️
...amplescraper/internal/metadata/generated_config.go	92.68%	2 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (58.21%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13431      +/-   ##
==========================================
- Coverage   92.44%   91.70%   -0.74%     
==========================================
  Files         630      630              
  Lines       35515    36141     +626     
==========================================
+ Hits        32832    33144     +312     
- Misses       2135     2422     +287     
- Partials      548      575      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dmitryax]

General approach sounds good

[rogercoll]

Code looks good to me, thanks for working on this 🚀

[dmitryax]

This line is redundant

[dmitryax]

After this call, we can loop over the attributes (using go templates), and, if we find an optional one, we do the same report call but with the optional attribute changed to let's say string_attr-val-2.

Then the expected value should be 1+<number of optional attributes>

[rogercoll]

@shalper2 Is there any reason for the attributes configuration being at root level instead of being configurable for each metric?

Disabling an attribute globally might lead to a user having to define multiple instances of a receiver if aggregation is needed just for a metric. For example, one might be interested in aggregating only in cpu states for utilization metrics:

    scrapers:
      cpu:
        metrics:
          system.cpu.time:  <- No aggregation, using default attributes definition attributes: [cpu, state]
            enabled: true
          system.cpu.utilization:
            enabled: true
            attributes: [cpu]  <- Override default attributes

The previous configuration looks more aligned with the actual metadata.yaml metrics definition:

  system.cpu.time:
    enabled: true
    description: ""
    unit: s
    sum:
      value_type: double
      aggregation_temporality: cumulative
      monotonic: true
    attributes: [cpu, state]

  system.cpu.utilization:
    enabled: false
    description: ""
    unit: "1"
    gauge:
      value_type: double
    attributes: [cpu, state]

Any thoughts on this?

[shalper2]

hey @rogercoll! The root level approach was considered mainly due to this thread. I think what you're proposing makes sense! I also think that having a set of attributes disabled at the root of the receiver is pretty clear and, while more verbose, easier to use in existing config.yaml files.

[rogercoll]

hey @rogercoll! The root level approach was considered mainly due to this thread. I think what you're proposing makes sense! I also think that having a set of attributes disabled at the root of the receiver is pretty clear and, while more verbose, easier to use in existing config.yaml files.

Thanks for sharing this. If I understand open-telemetry/opentelemetry-collector-contrib#16533 correctly, the idea was to move the attributes into the metric section. Attributes are only valuable when used/referenced by metrics.

Although enabling/disabling an attribute at the root level for all metrics might be valuable in some cases, I personally think that using the metrics attributes reference to configure the set of attributes for that metric will be more flexible and non-breaking. We won't need to add the enabled: configuration option to all current configurations, but the aggregation would rely on the default or configured attributes section within each metric. In that sense, a user won't need to do any change to their current configurations and won't need to duplicate the receiver instance if they just want to aggregate on one specific metric.

@shalper2 Do you think my suggestion would be feasible? Does it make the code generation process more complex?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.