-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
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.
App Arch updates LGTM
docs/development/plugins/data/public/kibana-plugin-plugins-data-public.querystringinput.md
Outdated
Show resolved
Hide resolved
loggers: HANDLED_IN_KIBANA_PLATFORM, | ||
root: HANDLED_IN_KIBANA_PLATFORM, | ||
|
||
silent: Joi.boolean().default(false), |
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.
optional: is it possible to migrate this to config-schema to remove another dependency on joi
?
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 thought it was not possible, TIL: we actually got siblingRef
in config-schema.
However the joi schema is still used by the legacy config: https://github.com/elastic/kibana/pull/77678/files#diff-30708bfadd0e62135e289c55c5d900be. Not sure we can convert it back to joi
to register it to the legacy config if we migrate it. We could use the protected internalSchema
property though I guess, but not sure this is a proper solution
kibana/packages/kbn-config-schema/src/types/type.ts
Lines 38 to 42 in c4ddd00
/** | |
* Internal "schema" backed by Joi. | |
* @type {Schema} | |
*/ | |
protected readonly internalSchema: AnySchema; |
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.
From what I can see: legacy platform doesn't read config values, but pass them to @kbn/legacy-logging
so it might be okay to move config validation to those @kbn/legacy-logging
methods that accept config as a function parameter. The main downside of this approach is that the logging config will be validated at a different time than the other config parts.
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.
Let's keep it that way until #81483 is resolved imho.
|
||
import KbnLoggerJsonFormat from './log_format_json'; | ||
import { attachMetaData } from './metadata'; | ||
import { createListStream, createPromiseFromStreams } from './test_utils'; |
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.
another place to fix when we move stream utils to @kbn/std
package
{ | ||
"name": "@kbn/legacy-logging", | ||
"version": "1.0.0", | ||
"private": true, |
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.
|
||
setupLogging(this, Config.withDefaultSchema(config)); | ||
setupLogging((this as unknown) as Server, loggingConfig, 2147483647); |
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 appender
kibana/src/core/server/legacy/logging/appenders/legacy_appender.ts
Lines 48 to 50 in ab92bbb
constructor(legacyLoggingConfig: Readonly<LegacyVars>) { | |
this.loggingServer = new LegacyLoggingServer(legacyLoggingConfig); | |
} |
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)
data: string | Record<string, any>; | ||
} | ||
|
||
export type AnyEvent = |
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.
🎉
return downgradeIfErrorType('HPE_INVALID_METHOD', event); | ||
} | ||
|
||
downgradeIfHTTPWhenHTTPS(event) { | ||
downgradeIfHTTPWhenHTTPS(event: any) { |
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.
any
--> unknown
?
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.
even better. any
-> AnyEvent
a60d262
* @param {Array<any>} items - the list of items to provide | ||
* @return {Readable} | ||
*/ | ||
export function createListStream<T = any>(items: T | T[] = []) { |
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.
we haven't move them to @kbn/std
yet 😞 let's do in a follow-up
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.
opened #84033
"declarationMap": true, | ||
"types": ["jest", "node"] | ||
}, | ||
"include": ["./src/**/*.ts"] |
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.
optional: src/**/*
💚 Build SucceededMetrics [docs]Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
* create kbn-legacy-logging package and start to move things * fix rotator tests * fix logging system test mocks * move logging format to the package * move logging setup to package * adapt legacy logging server * remove usage of legacy config in the legacy logging server * move legacy logging server to package * remove `??` syntax from package * update generated doc * fix a few things due to month old merge * remove typings from project * move reconfigureLogging to package * add basic README file * update generated doc * remove old typings * add typing for legacy logging events * remove `??` from packages * fix / improve event types usages * remove suffix from tsconfig # Conflicts: # src/legacy/server/config/schema.js
* create kbn-legacy-logging package (#77678) * create kbn-legacy-logging package and start to move things * fix rotator tests * fix logging system test mocks * move logging format to the package * move logging setup to package * adapt legacy logging server * remove usage of legacy config in the legacy logging server * move legacy logging server to package * remove `??` syntax from package * update generated doc * fix a few things due to month old merge * remove typings from project * move reconfigureLogging to package * add basic README file * update generated doc * remove old typings * add typing for legacy logging events * remove `??` from packages * fix / improve event types usages * remove suffix from tsconfig # Conflicts: # src/legacy/server/config/schema.js * fix for 7.x branch
* master: (67 commits) [Observability] Load hasData call asynchronously (elastic#80644) Implement AnonymousAuthenticationProvider. (elastic#79985) Deprecate `visualization:colorMapping` advanced setting (elastic#83372) [TSVB] [Rollup] Table tab not working with rollup indexes (elastic#83635) Revert "[Search] Search batching using bfetch (elastic#83418)" (elastic#84037) skip flaky suite (elastic#83772) skip flaky suite (elastic#69849) create kbn-legacy-logging package (elastic#77678) [Search] Search batching using bfetch (elastic#83418) [Security Solution] Refactor Timeline flyout to take a full page (elastic#82033) Drop use of console-stamp (elastic#83922) skip flaky suite (elastic#84011 , elastic#84012) Fixed usage of `isReady` for usage collection of alerts and actions (elastic#83760) [maps] support URL drilldowns (elastic#83732) Revert "Added default dedupKey value as an {{alertInstanceId}} to provide grouping functionality for PagerDuty incidents. (elastic#83226)" [code coverage] Update jest config to collect more data (elastic#83804) Added default dedupKey value as an {{alertInstanceId}} to provide grouping functionality for PagerDuty incidents. (elastic#83226) [Security Solution] Give notice when endpoint policy is out of date (elastic#83469) [Security Solution] Sync url state on any changes to query string (elastic#83314) [CI] Initial TeamCity implementation (elastic#81043) ...
Summary
Related to #76003 and #56205
Create the
@kbn/legacy-logging
package and move legacy logging implementation to it.Checklist