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

Remove global logger from outputs and monitoring #16761

Merged
merged 10 commits into from
Mar 5, 2020

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Mar 3, 2020

What does this PR do?

Remove global logger for libbeat/outputs and libbeat/monitoring.

Why is it important?

Precondition for improved structured logging.

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works

Author's Checklist

Related issues

Developer Docs

Following public methods are changed to take a logger as first parameter now:

  • common.transport.transport#ProxyDialer
  • common.transport.tlscommon.tls#ReadPEMFile
  • outputs.elasticsearch.client#BulkReadItemStatus

@simitt simitt requested a review from a team as a code owner March 3, 2020 14:53
@urso
Copy link

urso commented Mar 3, 2020

@ycombinator I think parts of this interfere with your refactoring.

@urso urso added refactoring review Team:Services (Deprecated) Label for the former Integrations-Services team labels Mar 3, 2020
@simitt
Copy link
Contributor Author

simitt commented Mar 3, 2020

@urso , @ycombinator - yes they do, but I was already done with the changes when I saw the other PR. Don't worry about it, I will just rebase my PR once the other one is merged.

@ycombinator
Copy link
Contributor

ycombinator commented Mar 3, 2020

Thanks @urso, @simitt. Pretty sure this PR here will go in before the refactoring one, given their relative sizes. So please go ahead here and I will incorporate your changes into my larger refactoring PR once this PR here is merged.

@simitt
Copy link
Contributor Author

simitt commented Mar 4, 2020

Failing tests seem unrelated.

Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

@simitt all CI failures are some know collateral from the switch to go modules. Related tests are all green.

Reading through the PR I found a many places where errors are logged with %v only. Please change these to use %+v. Zap does not handle errors well or in an uniform manner, especially if errors are nested. In case of nested errors there is a high chance that the cause is not logged when using %v. Some error types implement a custom fmt.Formatter, and will only print the cause if the + flag is parsed.

@simitt simitt added the release-note:dev_docs Contains a Dev Docs section label Mar 4, 2020
@urso
Copy link

urso commented Mar 4, 2020

LGTM.

Thank you for taking the time and doing so many changes at once!

@urso urso added the needs_backport PR is waiting to be backported to other branches. label Mar 4, 2020
@simitt
Copy link
Contributor Author

simitt commented Mar 5, 2020

@urso I rebased my PR on top of the changes of #16734, can you please re-review the changes from the last two commits.

Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs_backport PR is waiting to be backported to other branches. refactoring release-note:dev_docs Contains a Dev Docs section review Team:Services (Deprecated) Label for the former Integrations-Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants