-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
mdatagen: add wildcard name matching for configs #10065
Conversation
026ad18
to
2ed7ec4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10065 +/- ##
==========================================
- Coverage 92.59% 92.43% -0.16%
==========================================
Files 387 387
Lines 18198 18264 +66
==========================================
+ Hits 16850 16883 +33
- Misses 1007 1031 +24
- Partials 341 350 +9 ☔ View full report in Codecov by Sentry. |
I think that code coverage change is fine here because none of the |
The changelog issue can only be resolved once the issue is moved from |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@braydonk, are you still working on this? Just checking. Let me know if you have any concerns with my comments |
Hi @dmitryax yes I am. I realized that I forgot to mention that I am writing a full spec to explain how it works, and while I was doing that found a number of edge cases that prompted me to rewrite the parser. Taking longer than expected. I am still working on this though and will keep it unstale. |
This PR adds support for wildcard name matching. Generated MetricsConfig and ResourceAttributesConfig will now support providing names not just as full names, but also using `*` wildcards and multimatching with `{x,y,etc}`. This allows you to apply configs to groups of metrics and resource attributes, simplifying configs. Signed-off-by: braydonk <[email protected]>
What the actual errors look like from collector output is not super obvious from the PR, so here are some examples. I used the hostmetrics process scraper in my example. Not matching any names with the patterns hostmetrics:
collection_interval: 10s
scrapers:
process:
metrics:
"nothing.*":
enabled: false
Parsing errors in a pattern hostmetrics:
collection_interval: 10s
scrapers:
process:
metrics:
"a_.*":
enabled: false
|
I'm gonna wait to address the conflicts, cause this is just going to keep coming up continuously throughout the review. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Not stale |
@@ -10,6 +10,12 @@ import ( | |||
) | |||
{{ if .Metrics -}} | |||
|
|||
var MetricNames = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to expose this variable if it's not intended to be used outside. It can be rendered even right in expandPatternMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch thanks, this was an accident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a758241
I couldn't render directly in expandPatternMap
because there could be either metrics or resource attributes passed in. I render both arrays more locally to their usage though instead of as globals.
// or end of input are valid. | ||
if !p.isFinished() { | ||
curr, _ := p.current() | ||
if curr != '.' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we allow a group as a whole to be either an identifier, a group, or a wildcard, can we use strings.Split(pattern, '.')
and just read each part instead of having this loop? I think it should simplify the code.
I like the approach you took. However, even if we do the parsing ourselves, I'd like to keep the code simpler if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give that a shot. When I first tried that I was having trouble making nice error messages; currently the error messages can track the exact token that is causing a problem. I think I can come up with a way to preserve that though, will give it a try soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this but I found it was becoming equally as complex. The complexity saved by this approach is:
- Shared scanner object, so I'm not needing to pass around indexes for context to different spots
- I can share identifier parsing code between an identifier as a root group, and an identifier as part of a multimatch
How strongly do you feel on this? I can try to make it work if you think it's the way to go.
cmd/mdatagen/main.go
Outdated
@@ -124,6 +124,23 @@ func run(ymlPath string) error { | |||
toGenerate[filepath.Join(tmplDir, "resource.go.tmpl")] = filepath.Join(codeDir, "generated_resource.go") | |||
toGenerate[filepath.Join(tmplDir, "resource_test.go.tmpl")] = filepath.Join(codeDir, "generated_resource_test.go") | |||
} | |||
if err = generateFile(filepath.Join(tmplDir, "match.go.tmpl"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be generated for every component in contrib. We need to generate these files only for scrapers len(md.Metrics) > 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f416166
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction in a522523
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description
This PR adds support for wildcard name matching.
Generated MetricsConfig and ResourceAttributesConfig will now support providing names not just as full names, but also using
*
wildcards and multimatching with{x,y,etc}
. This allows you to apply configs to groups of metrics and resource attributes, simplifying configs.Link to tracking issue
Fixes #10074
Testing
I
go install
ed mdatagen from this PR and ran it on a receiver inopentelemetry-collector-contrib
and was able to use wildcard matching to enable/disable groups of metrics as expected.Documentation
I have written a specification in a gist: https://gist.github.com/braydonk/ccb6775331fdd5dca91a650330b9839f
I am not sure where this documentation should live.