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

Removing beta label from jolokia/jmx metricset #6143

Merged
merged 1 commit into from
Mar 13, 2018

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jan 23, 2018

No description provided.

@ruflin ruflin added in progress Pull request is currently in progress. module Metricbeat Metricbeat labels Jan 23, 2018
if logp.IsDebug(metricsetName) {
debugf("The body for POST requests to jolokia host %v is: %v",
base.HostData().Host, string(body))
log.Debugw("Jolokia request body",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh Is this the expected use of the new logging (first time user ;-) ).

Copy link
Member

Choose a reason for hiding this comment

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

You can omit host if you add it to the logger's context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -69,16 +62,19 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
http.SetMethod("POST")
http.SetBody(body)

log := logp.NewLogger(metricsetName)
Copy link
Member

Choose a reason for hiding this comment

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

Given that there can be multiple instances of this metricset I recommend instantiating this Logger with context about the host. For example:

log := logp.NewLogger(metricsetName).With("host", base.HostData().Host)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if logp.IsDebug(metricsetName) {
debugf("The body for POST requests to jolokia host %v is: %v",
base.HostData().Host, string(body))
log.Debugw("Jolokia request body",
Copy link
Member

Choose a reason for hiding this comment

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

You can omit host if you add it to the logger's context.

@@ -69,16 +62,19 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
http.SetMethod("POST")
http.SetBody(body)

log := logp.NewLogger(metricsetName)

if logp.IsDebug(metricsetName) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh Do we have any direct replacment method for IsDebug?

Copy link
Member

Choose a reason for hiding this comment

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

Nope. Calling IsDebug is mostly unnecessary when using the structured logger because the fields are lazily marshaled to a log message. And for the print and printf style methods the string formatting is not performed until after checking if the logging level is enabled. So the only performance hit is in the allocation of the slice that holds the fields.

@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels Feb 6, 2018
@ruflin
Copy link
Contributor Author

ruflin commented Feb 12, 2018

@andrewkroh Ready for an other review. Changed the logging, added a Changelog entry and rebase on master.

@ruflin
Copy link
Contributor Author

ruflin commented Mar 13, 2018

Rebased again.

@andrewkroh andrewkroh merged commit b4783c9 into elastic:master Mar 13, 2018
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.

2 participants