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

Allow loggers to create child loggers via 'get' method #52605

Merged
merged 7 commits into from
Dec 16, 2019

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Dec 10, 2019

Summary

Fix #39695

Avoid having to propagate both Logger and LoggerFactory in nested parts of the code by adding a get method on Logger that will creates a child logger from it's context.

const logger = loggerFactory.get('plugin', 'service'); // 'plugin.service' context
const subLogger = logger.get('feature'); // 'plugin.service.feature' context

PR also adds a createLogger method to the logging service mock, and uses it in some plugin tests that were manually declaring a mocked Logger

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@pgayvallet pgayvallet added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0 labels Dec 10, 2019
@pgayvallet pgayvallet changed the title Add factory get method to Logger Allow loggers to create child loggers via 'get' method Dec 10, 2019
@pgayvallet pgayvallet marked this pull request as ready for review December 10, 2019 10:07
@pgayvallet pgayvallet requested review from a team as code owners December 10, 2019 10:07
@pgayvallet pgayvallet added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Dec 10, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Finally! Spaces changes LGTM.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

💯

warn: jest.fn(),
get: jest.fn(),
};
mockLog.get.mockImplementation((...ctx) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: use context for consistency with logging service mock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

context is already declared in the upper scope (no-shadow) 😢

@spalger
Copy link
Contributor

spalger commented Dec 13, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit 408139b into elastic:master Dec 16, 2019
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Dec 16, 2019
* add 'Logger.get' method

* updates generated doc

* resolve merge conflicts
pgayvallet added a commit that referenced this pull request Dec 16, 2019
* add 'Logger.get' method

* updates generated doc

* resolve merge conflicts
timductive pushed a commit to timductive/kibana that referenced this pull request Dec 16, 2019
* add 'Logger.get' method

* updates generated doc

* resolve merge conflicts
@tsullivan tsullivan mentioned this pull request Jan 6, 2020
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger in New Platform should expose a factory method
5 participants