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

feat: add procfs cmdline #2799

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

dreamerlzl
Copy link
Contributor

@dreamerlzl dreamerlzl commented May 17, 2024

Why?

Add an extra label for distinguishing processes, especially for java processes where the comms are the same.

What?

Add a procfs label cmdline from /proc/[pid]/cmdline

How?

The code is short and self-explanatory.

Test Plan

See the modified integration test.

@dreamerlzl dreamerlzl requested a review from a team as a code owner May 17, 2024 08:45
@dreamerlzl
Copy link
Contributor Author

dreamerlzl commented May 18, 2024

Wondering whether the profiler timeouts for Python and Java are not long enough... seeing similar check failures in other PRs.

@brancz
Copy link
Member

brancz commented May 20, 2024

I'm worried that this can easily expose secrets in profiling data. I think we should only expose this with a flag on that is off by default.

We're working on fixing the tests, they are knowingly broken at the moment.

@@ -184,7 +184,8 @@ type FlagsMetadata struct {
ExternalLabels map[string]string `help:"Label(s) to attach to all profiles."`
ContainerRuntimeSocketPath string `help:"The filesystem path to the container runtimes socket. Leave this empty to use the defaults."`

DisableCaching bool `default:"false" help:"Disable caching of metadata."`
DisableCaching bool `default:"false" help:"Disable caching of metadata."`
EnableProcessCmdline bool `default:"false" help:"add /proc/[pid]/cmdline as a label."`
Copy link
Member

Choose a reason for hiding this comment

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

"add" -> "Add" and please add a note about the potential security implications.

@brancz brancz merged commit 5fd0da8 into parca-dev:main May 20, 2024
11 of 12 checks passed
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.

None yet

3 participants