Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[processor/metricsgeneration] Implement attribute matching functionality #35628

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

crobert-1
Copy link
Member

Description

The metrics generation processor's default behavior is to get the first data point's value in the configured metric2, and use it for all calculations done with the given rule. As described in the issue, this may yield unexpected or incorrect results.

This introduces the feature gate metricsgeneration.MatchAttributes. When enabled, calculations will only be done on data points whose overlapping attributes match. This means that if both data points have a given attribute, their values must match.

Link to tracking issue

Fixes #35425

Testing

Added golden tests to ensure expected metrics are generated. Existing tests remain unchanged, showing this functionality did not impact other functions.

pmetrictest.IgnoreStartTimestamp(),
pmetrictest.IgnoreTimestamp())
assert.NoError(t, err)
t.Run(testCase.name, func(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified this test's setup so each test case can be run independently locally, and the test name in failures will be more obvious. The actual test functionality here remains largely unchanged, other than setting the feature gate value.


// generateScalarMetrics creates a new metric based on a scalar type rule and adds it to the scope metric.
// The value for newly calculated metrics is always a floating point number.
func generateScalarMetrics(rm pmetric.ResourceMetrics, operand2 float64, rule internalRule, logger *zap.Logger) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is the original generateMetrics function, renamed to make the differing behavior obvious.

newMetric.MoveTo(metric)
// Append the new metric to the scope metrics. This will only append the new metric if it
// has data points.
func appendNewMetric(ilm pmetric.ScopeMetrics, newMetric pmetric.Metric, name, unit string) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic of only adding new metrics if they have valid data points was going to be copied in multiple places, so I decided to move it here to a centralized location.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've attempted to make attribute values and data point values obviously different, to help scan the expected results for correctness. The important part is to make sure all original attributes are copied over (merging the attributes of metric 1 and metric 2), and to ensure the "overlapping attributes" functionality is handled properly.

@atoulme atoulme force-pushed the metricsgeneration_match_attrs branch from 6fea4bc to 2bf3cea Compare October 11, 2024 17:56
@atoulme
Copy link
Contributor

atoulme commented Oct 11, 2024

I just rebased off main to fix go.mod/go.sum conflicts.

@dmitryax dmitryax merged commit 6b5e73b into open-telemetry:main Oct 15, 2024
158 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/metricsgeneration Metrics Generation processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[processor/metricsgeneration] Add option to match metric attributes for calculation operations
4 participants