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

Add metric filter option to metrics transform processor #1447

Merged

Conversation

james-bebbington
Copy link
Member

@james-bebbington james-bebbington commented Nov 2, 2020

Link to tracking Issue: #1088 (comment)

Description:

Add a simple include filter option with strict or regexp match type so that transforms can be applied to a range of metrics. When using the regexp match type, new_name supports expansion.

This implementation is compatible with the config proposed here so could be switched out for that implementation in the future if desired. Also retained the metric_name config option for backwards compatibility but removed references to this from the documentation.

Continues optimizing for the strict match case, i.e. retain O(n) + O(t) for strict matches where metric names are unique, at the cost of making regexp matching slightly slower. regexp matches will be O(n) * O(t).

Also resolves an edge case where if multiple metrics have the same name only one of them would previously have been transformed.

Documentation:

Updated the documentation to specify how to use the match_type filter, and generally cleaned up the README to be more easily digested.

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #1447 into master will increase coverage by 17.92%.
The diff coverage is 98.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1447       +/-   ##
===========================================
+ Coverage   70.87%   88.80%   +17.92%     
===========================================
  Files          29      341      +312     
  Lines        1298    16690    +15392     
===========================================
+ Hits          920    14822    +13902     
- Misses        321     1403     +1082     
- Partials       57      465      +408     
Flag Coverage Δ
integration 70.80% <ø> (-0.08%) ⬇️
unit 88.08% <98.50%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
processor/metricstransformprocessor/factory.go 98.76% <96.66%> (ø)
...stransformprocessor/metrics_transform_processor.go 100.00% <100.00%> (ø)
receiver/awsecscontainermetricsreceiver/factory.go 100.00% <0.00%> (ø)
exporter/sentryexporter/utils.go 100.00% <0.00%> (ø)
receiver/k8sclusterreceiver/collection/cronjobs.go 100.00% <0.00%> (ø)
processor/groupbytraceprocessor/event.go 96.61% <0.00%> (ø)
...ceiver/k8sclusterreceiver/collection/daemonsets.go 94.28% <0.00%> (ø)
...ver/kubeletstatsreceiver/kubelet/stats_provider.go 55.55% <0.00%> (ø)
...r/alibabacloudlogserviceexporter/trace_exporter.go 75.00% <0.00%> (ø)
...ter/signalfxexporter/dimensions/dimensionupdate.go 90.90% <0.00%> (ø)
... and 321 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68d65d7...b4701ee. Read the comment docs.

processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/metricstransformprocessor/factory.go Outdated Show resolved Hide resolved
processor/metricstransformprocessor/factory.go Outdated Show resolved Hide resolved
}

func (f internalFilterRegexp) getMatches(toMatch metricNameMapping) []*metricspb.Metric {
const capExpectedMatches = 10
Copy link
Member

Choose a reason for hiding this comment

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

Any reasoning for this value?

Copy link
Member Author

@james-bebbington james-bebbington Nov 3, 2020

Choose a reason for hiding this comment

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

Just seemed like a sensible default. If a user is using regexp match type, they probably expect more than 1 match.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Perhaps add this as a comment to the code? Otherwise, it's still going to be a magic number :)

if transform.Action == Update {
nameToMetricMapping.remove(metricName, metric)
}
nameToMetricMapping.add(metric.MetricDescriptor.Name, metric)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be transform.NewName?

Copy link
Member Author

@james-bebbington james-bebbington Nov 3, 2020

Choose a reason for hiding this comment

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

The metric name has been transformed at this point so either would work.

I wanted to use metric.MetricDescriptor.Name here so this works even if transform.NewName supports expansion (and thus the new name can differ from the raw value of transform.NewName).

I've now added expansion to this PR - see comment below.

@james-bebbington
Copy link
Member Author

@jpkrohling FYI while addressing your comments, I also added one minor feature to this PR, which is that if the match_type is regexp, then new_name now supports regexp expansion, see the updated test case.

@bogdandrutu bogdandrutu merged commit e3e304a into open-telemetry:master Nov 3, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
* TraceContext propagator now handles `tracestate`.

Signed-off-by: Anthony J Mirabella <[email protected]>

* Prevent invalid tracestate from invalidating traceparent.

Signed-off-by: Anthony J Mirabella <[email protected]>

* Fail tests early if unable to construct expected TraceContext

Signed-off-by: Anthony J Mirabella <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants