Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
create kbn-legacy-logging package #77678
create kbn-legacy-logging package #77678
Changes from 12 commits
3ad4c93
73122e5
87ca167
f6dd86c
ba67eb7
e2e1615
bd0c68d
76f4f27
e54b7d9
361deda
5d7b70a
6bd16cf
8ddc262
f6c67de
8382a09
56e3ea1
4b8a661
5f0471d
294a05a
f2766e5
0dd3b23
5a37913
a60d262
dbbe2e5
389dfe9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a Readme that this package is a temporary workaround until we migrate logging to the KP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more planning to keep it there until we totally get rid of the legacy logging system in 8.0 tbh. It will be way easier to delete it with all the code being centralized in this package. Was that what you meant?
edit: oh, I guess you mean the server event logging part. But there is the same issue. We have to keep support of the legacy logging format, meaning that the module won't go away before 8.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we expect
LegacyLoggingServer
won't be called when the package is imported from CLI scripts? How does the final dependency tree look like?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only import of this package in core (soon in
kbn/config
instead) is from the legacy appenderkibana/src/core/server/legacy/logging/appenders/legacy_appender.ts
Lines 48 to 50 in ab92bbb
The idea was to change the file to use a dynamic
require
in the constructor to only load the module when needed. The cli / packages that will use the config service would default to KP logging (probably just defaulting to console appender), that way, the legacy logging (and hapi stuff) is not imported.Note that the main goal to not load the legacy logging system / hapi when importing the config service was for the apm script instrumentation, which is no longer concerned by the problem, as it will have its own loader to ensure we are not importing any module we want to instrument at all (#77855)