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 config option to disable or enable cmdline cache for System Process Metricset #3891

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

martinscholz83
Copy link
Contributor

fixes #3799

This PR add a config option for the process metricset to disable or enable caching the commandline args for a process.

@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

In general LGTM. I thinking if this cache should be emptied from time to time (timeout) or on / off is enough?

This metricset caches the command line args for a running process. This means if you alter
the command line for a process while this metricset is running, these changes are not detected.
This feature can disabled by adding
`process.cmdline.cache: false` to the system module configuration.
Copy link
Member

Choose a reason for hiding this comment

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

We should add this option also to the full config file: https://github.com/elastic/beats/blob/master/metricbeat/module/system/_meta/config.full.yml (don't forget make update ;-) )

Copy link
Contributor Author

@martinscholz83 martinscholz83 Apr 4, 2017

Choose a reason for hiding this comment

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

Do you mean to add an optional timeout config if cache is true

process.cmdline.cache: true
process.cmdline.timeout: 5m

Copy link
Member

Choose a reason for hiding this comment

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

I was more thinking out loud. So far I don't think we had any requests for it so I would leave it out. But it is good to have it as a potential option for the future.

Copy link
Member

Choose a reason for hiding this comment

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

When the PID disappears the cached cmdline for the PID is dropped. This is the current behavior.

@ruflin
Copy link
Member

ruflin commented Apr 4, 2017

Could you also update the changelog?

@martinscholz83
Copy link
Contributor Author

Ok. Is the grammar of the description right?


This metricset caches the command line args for a running process. This means if you alter
the command line for a process while this metricset is running, these changes are not detected.
This feature can disabled by adding
Copy link
Member

Choose a reason for hiding this comment

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

This feature can be disabled by adding

@ruflin
Copy link
Member

ruflin commented Apr 4, 2017

@maddin2016 Left one additional comment there. Would also like to get @andrewkroh opinion on the feature.

@andrewkroh
Copy link
Member

The implementation LGTM. I have two questions.

  • @ruflin I'm wondering if the config value should have "enabled" in it since it's boolean, like process.cmdline.cache.enabled: true/false. I don't have an opinion either way.
  • @maddin2016 Is it possible for an application to change its command line on Windows?

@ruflin
Copy link
Member

ruflin commented Apr 4, 2017

@andrewkroh The nice part about having enabled inside is that we get some "free magic" from go-ucfg so I'm +1 on enabled.

@ruflin
Copy link
Member

ruflin commented Apr 4, 2017

An other advantage of adding enabled is that in case we add a timeout in the future, process.cmdline.cache.timeout: 5m is possible and I think makes more sense.

@martinscholz83
Copy link
Contributor Author

Is it possible for an application to change its command line on Windows?

Of course
image

@andrewkroh
Copy link
Member

Do you happen to know what API call an application needs to use to update its command line after starting? On Linux this is possible (I think via magic) and BSD has setproctitle. But I can't find anything about this for Win.

@martinscholz83
Copy link
Contributor Author

See here
But!!

The ChangeServiceConfig function changes the configuration information for the specified service in the service control manager database. You can obtain the current configuration information by using the QueryServiceConfig function.
If the configuration is changed for a service that is running, with the exception of lpDisplayName, the changes do not take effect until the service is stopped.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for implementing this.

@andrewkroh andrewkroh merged commit 30d9baa into elastic:master Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metricbeat process module cmdline does not update even though process alters it
4 participants