-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(inputs.procstat): Add child level tag #16105
base: master
Are you sure you want to change the base?
Conversation
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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.
Thanks for the PR! I have one code question, but also does this close issue #16104? If so could you update the PR description to close the issue? Thanks!
@@ -202,6 +202,7 @@ func (f *Filter) ApplyFilter() ([]processGroup, error) { | |||
tags[k] = v | |||
} | |||
tags["parent_pid"] = strconv.FormatInt(int64(p.Pid), 10) | |||
tags["child_level"] = strconv.FormatInt(int64(depth), 10) |
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 think this should be a configuration option since it is a new tag
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 agree as it will also likely produce high cardinality. How about adding this as a field instead?
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 think the cardinality is higher on the parent_pid field so this change is less impactful than one might think.
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.
In any case, we cannot add a new tag unconditionally...
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.
Okay, I understand your point of view.
What option would you suggest to add this tag?
Is there a naming rule?
It also means that for each plugin each time new tags are added, an option is needed. Is this likely to create plugins with long configurations?
We could put:
tag_child_level.
If not defined, the tag is not added.
If tag_child_level=toto then the level is added in the tag "toto"
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.
up ? ;-)
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 think this should be a configuration option since it is a new tag
@DStrand1 Have you a name recommanded for activate new tag ?
Yes it's the same. The issue is in the PR description. What do you understand exactly by :
|
Summary
Add tag for children process to be able to have information by level
Checklist
Related issues
resolves #16104