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

Implement standard compatible severity formatting to LevelFluentLogger #57

Merged
merged 3 commits into from
Jan 16, 2017

Conversation

saidie
Copy link
Contributor

@saidie saidie commented Nov 15, 2016

Hi,

The recently added standard logger compatible LevelFluentLogger is great!
I've been tried to use it in my project and noticed that its use of formatter is incompatible with standard one.

That is, format_severity, which is a private method of ::Logger, is used inside the formatter here. This could be simply because format_severity is not applied here.
In the standard ::Logger implementation, format_severity is applied before passing a severity to formatter like this.

This PR implements standard log formatter compatible way of severity formatting.

@saidie saidie changed the title Implement standard compatible severity formatting to LevelFluentLogger Implement standard compatible severity formatting to LevelFluentLogger Nov 15, 2016
@sonots
Copy link
Member

sonots commented Nov 15, 2016

LGTM. @toyama0919 ping?

@saidie
Copy link
Contributor Author

saidie commented Nov 15, 2016

Thank you!
Now, I'm trying to fix the test 🤔

@saidie
Copy link
Contributor Author

saidie commented Nov 15, 2016

It seems to fail due to recent master branch updates of https://github.com/fluent/fluentd

@toyama0919
Copy link
Contributor

The recently added standard logger compatible LevelFluentLogger is great!

😊

LGTM.

@saidie
Copy link
Contributor Author

saidie commented Nov 15, 2016

Thank you for LGTM 😊

@saidie
Copy link
Contributor Author

saidie commented Nov 15, 2016

By manual binary search, I found that the test failure is caused by this fluentd PR.
And all test cases are passed in my machine when WAIT environment variable has longer value, e.g., 0.5. Maybe we need to make ENV['WAIT'] default value longer as with the similar commit in the PR ?

@repeatedly
Copy link
Member

That's weird. It means the response time of new in_forward is longer than old in_forward?

@saidie
Copy link
Contributor Author

saidie commented Nov 15, 2016

I'm not well understood the detail of the fluentd PR but it sounds weird too that new plugin is slower.
ENV['WAIT'] is used also for waiting dummy fluentd startup here. This could be a core of the problem ??

@repeatedly
Copy link
Member

This could be a core of the problem ??

If increasing wait time resolves test failures,
it is one temporarily approach.

@saidie
Copy link
Contributor Author

saidie commented Nov 15, 2016

By my investigation, the problem is that the posted content has not yet been read by in_forward plugin when DummyFluentd#output is called. This is occurred because Cool.io event loop takes some time here (about 0.5s in my machine).
It seems to be inevitable to increase wait time at this point. I will create another PR for that tomorrow (or could I include it in this PR?).

@repeatedly
Copy link
Member

Sorry for the late. Just merged.

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.

4 participants