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/cumulativetodelta] Add include/exclude configuration #8952

Merged
merged 34 commits into from
Apr 21, 2022

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Mar 29, 2022

Description:

This PR replaces the existing metrics configuration option in favor of a more robust include/exclude system. The new configuration allows users to specify, either by name or by regex pattern, which metrics should or should not be converted. If neither include or exclude are supplied, all metrics appropriate metrics will be converted. If a name matches both an include and exclude rule, exclude will take precedence.

To support existing customers, the metrics configuration is still supported. The metrics configuration cannot be used with include/exclude. When metrics is used, a warning will be logged during startup.

This PR only updates the configuration options used to determine which metrics should be converted. The underlying convert logic has not been changed.

Link to tracking Issue:
Fixes #5877

Testing:
Unit tests were updated/added. Also tested new configuration locally using hostmetrics receiver.

Documentation:
Updated the processor's readme with the new configuration and more examples. Also clarified that non-monotinic sums are never converted.

@TylerHelmuth
Copy link
Member Author

@Aneurysm9 it appears the link job is hung, can you re-trigger it?

@jpkrohling
Copy link
Member

PR updated to address the v0.48.0 release.

@TylerHelmuth
Copy link
Member Author

@open-telemetry/collector-contrib-approvers please review.

CHANGELOG.md Outdated Show resolved Hide resolved
processor/cumulativetodeltaprocessor/config.go Outdated Show resolved Hide resolved
TylerHelmuth and others added 2 commits April 13, 2022 09:17
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Only one last thing and it LGTM (sorry for the slow drip of comments :)): can you add a test for the regexp match type?

@TylerHelmuth
Copy link
Member Author

Only one last thing and it LGTM (sorry for the slow drip of comments :)): can you add a test for the regexp match type?

There are some tests in processor_test.go that verify that regexp is useable, but I can also add a test to config_test.go

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Yes, I meant on config_test.go :). This LGTM, but I will wait for any comments by @Aneurysm9 before marking as ready to merge

@TylerHelmuth
Copy link
Member Author

@Aneurysm9 please review

Copy link
Contributor

@pmm-sumo pmm-sumo left a comment

Choose a reason for hiding this comment

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

I like that it now uses filterset, which brings more consistency to filtering data across the processors. There are some minor conflicts to resolve but otherwise it LGTM

@mx-psi
Copy link
Member

mx-psi commented Apr 19, 2022

I will wait a couple more days for Anthony and mark as ready to merge then (ping me if I forget)

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cumulativetodeltaprocessor] Improve metric name configuration
7 participants