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

Support log level setting per plugin #268

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

okkez
Copy link

@okkez okkez commented Nov 22, 2018

No description provided.

@jkohen
Copy link
Contributor

jkohen commented Aug 12, 2019

@qingling128 what does it take to resolve this PR?

@qingling128
Copy link
Contributor

Looks like it needs rebasing. I tried the Github UI tool, but there is still a test failure. @okkez - Mind rebasing this off the latest master if it's still needed?

@okkez
Copy link
Author

okkez commented Aug 15, 2019

Rubocop causes CI failure. I've fixed it.
It is necessary to configure the log level per plugin.

@@ -1132,15 +1130,15 @@ def set_vm_id
@vm_id ||= fetch_gce_metadata('instance/id') if @platform == Platform::GCE
@vm_id ||= ec2_metadata['instanceId'] if @platform == Platform::EC2
rescue StandardError => e
@log.error 'Failed to obtain vm_id: ', error: e
log.error 'Failed to obtain vm_id: ', error: e
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: How did we decide which cases are log, and which cases are $log?

Copy link
Member

Choose a reason for hiding this comment

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

It's hidden in the extra part of the commit message on 63387b5. Frankly, I'd prefer it as a code comment in configure.

Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

@okkez, can you please expand on what exactly this PR accomplishes? Frankly, I don't see how the commits relate to the PR title. Is there a real-world problem this solves? Is there a best practices document we can look at to justify these changes?

@@ -1132,15 +1130,15 @@ def set_vm_id
@vm_id ||= fetch_gce_metadata('instance/id') if @platform == Platform::GCE
@vm_id ||= ec2_metadata['instanceId'] if @platform == Platform::EC2
rescue StandardError => e
@log.error 'Failed to obtain vm_id: ', error: e
log.error 'Failed to obtain vm_id: ', error: e
Copy link
Member

Choose a reason for hiding this comment

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

It's hidden in the extra part of the commit message on 63387b5. Frankly, I'd prefer it as a code comment in configure.

@okkez
Copy link
Author

okkez commented Aug 21, 2019

$log is a global variable, so we cannot control its log level per plugin, we can control it in global.
In the configure method, we have to use $log to output logs, because the logger instance (we use log method to fetch it) per plugin has not been configured while invoking configure method.

AFAIK, there is no document about $log and log.
But I can find the document to understand the log level per plugin.
See https://docs.fluentd.org/deployment/logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants