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 parameter support to telemetry devices #458

Merged

Conversation

danielmitterdorfer
Copy link
Member

With this commit we add a new command line flag --telemetry-params
which allows users to inject parameters to telemetry devices.

We also enhance the jfr telemetry device to choose the recording
template based on such a parameter.

With this commit we add a new command line flag `--telemetry-params`
which allows users to inject parameters to telemetry devices.

We also enhance the `jfr` telemetry device to choose the recording
template based on such a parameter.
@danielmitterdorfer danielmitterdorfer added enhancement Improves the status quo :Telemetry Telemetry Devices that gather additional metrics labels Apr 5, 2018
@danielmitterdorfer danielmitterdorfer added this to the 0.10.0 milestone Apr 5, 2018
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few minor comments. New tests worked fine.


java_opts = self.java_opts(log_file)

logger.info("jfr: Adding JVM arguments: [%s].", java_opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use the new .format() here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah silly me. It's the logger. Ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a different case here and I wanted to discuss this separately anyway. It looks as if we are using old style string formatting but we are actually not. This is the standard way the Python logging facility is used and I think it would make sense that we go with the default here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll merge then. :)

@danielmitterdorfer danielmitterdorfer merged commit 3e3e8ce into elastic:master Apr 6, 2018
@danielmitterdorfer danielmitterdorfer deleted the telemetry-params branch April 6, 2018 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :Telemetry Telemetry Devices that gather additional metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants