Skip to content

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

Merged
pgayvallet merged 7 commits into
elastic:masterfrom
pgayvallet:kbn-39695-logger-factory
Dec 16, 2019
Merged

Allow loggers to create child loggers via 'get' method#52605
pgayvallet merged 7 commits into
elastic:masterfrom
pgayvallet:kbn-39695-logger-factory

Conversation

@pgayvallet
Copy link
Copy Markdown
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 Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// label Dec 10, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

spalger commented Dec 13, 2019

@elasticmachine merge upstream

@pgayvallet pgayvallet force-pushed the kbn-39695-logger-factory branch from bbfc591 to 3f2c557 Compare December 16, 2019 07:05
@elasticmachine
Copy link
Copy Markdown
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
@tsullivan tsullivan mentioned this pull request Jan 6, 2020
19 tasks
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
* add 'Logger.get' method

* updates generated doc

* resolve merge conflicts
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 Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// 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