Closed lukeelmers closed 2 years ago
Pinging @elastic/kibana-core (Team:Core)
ack
Edited the package names to fit the conventions.
Note: we will encounter our first reverse dependency between internal and public stuff here, as some of our public types are inferred from public and/or internal schemas:
atm stuff like appendersSchema
is flagged as public and exported from the src/core/server
entrypoint. To be honest, I don't fully remember if that's because we really have consumers of this thing (unlikely) or if it was only exported to please the API extractor that need it to infer some other public types.
We have multiple options to address that:
@kbn/core-logging-server
)TypeOf<typeof schema>
(preferred if we only expose those schema publicly for documentation purposes)PR: https://github.com/elastic/kibana/pull/134438
Commit: https://github.com/elastic/kibana/pull/134438/commits/e82110a7fdece01ccb51fc2dd7e073c456ba131c
I look at the logging service's setup/start contract to see which types are publicly exposed
We only have a public setup contract for the LoggingService
: the LoggingServiceSetup
LoggingServiceSetup
only have one API exposed: configure(config$: Observable<LoggerContextConfigInput>): void;
The only (root) type we need to check is therefor LoggerContextConfigInput
Looking at the type:
export interface LoggerContextConfigInput {
// config-schema knows how to handle either Maps or Records
appenders?: Record<string, AppenderConfigType> | Map<string, AppenderConfigType>;
loggers?: LoggerConfigType[];
}
I see we're using two subtypes: AppenderConfigType
and LoggerConfigType
LoggerConfigType
was inferred from our config schemaexport type LoggerConfigType = TypeOf<typeof loggerSchema>;
it's a rather easy type though, given it doesn't use nested type, so I rewrite it to an explicit interface
export interface LoggerConfigType {
appenders: string[];
name: string;
level: LogLevelId;
}
AppenderConfigType
I see that the definition is here:
export type AppenderConfigType =
| ConsoleAppenderConfig
| FileAppenderConfig
| RewriteAppenderConfig
| RollingFileAppenderConfig;
However all the nested types are already explicitly defined, without using TypeOf
, e.g
export interface ConsoleAppenderConfig {
type: 'console';
layout: LayoutConfigType;
}
So I don't have to do anything, it's just that these types will all have to move into the public package
At this point I know I will need the following packages
@kbn/core-logging-server
The public types package. I know I need it because the logging
service has at least one public contract
This will contain the following types:
LoggingServiceSetup
LoggerContextConfigInput
LoggerContextConfigInput
@kbn/core-logging-server-internal
The concrete implementation's package. As long as we have a service implementation, I know I need one
@kbn/core-logging-server-mocks
I see we have mocks in src/core/server/logging
:
src/core/server/logging/logging_service.mock.ts
src/core/server/logging/logging_system.mock.ts
So we will have to create a mock package
Commit: https://github.com/elastic/kibana/pull/134438/commits/b78c0fb20ad91816da168ee42426eb79723dc511
I use the scripts/generate package
script for that.
First, I confirm that I'm only creating server-side packages here. I am, so I will not use the --web
option
mkdir packages/core/logging
node scripts/generate package --dir ./packages/core/logging/core-logging-server @kbn/core-logging-server
node scripts/generate package --dir ./packages/core/logging/core-logging-server-internal @kbn/core-logging-server-internal
node scripts/generate package --dir ./packages/core/logging/core-logging-server-mocks @kbn/core-logging-server-mocks
git add *
if clean repo, or just use the IDE for that
yarn run kbn bootstrap
-> confirm that yarn.lock
has been updated
@kbn/core-logging-server
Commit: https://github.com/elastic/kibana/pull/134438/commits/afa4234ddfdcbd86030ee3b3c69400dbb01e6c1c
I move LoggingServiceSetup
and LoggerContextConfigInput
to packages/core/logging/core-logging-server/src/contracts.ts
I move LoggerConfigType
to packages/core/logging/core-logging-server/src/logger.ts
I move AppenderConfigType
and the underlying appender types to packages/core/logging/core-logging-server/src/appenders/*
I move LayoutConfigType
and the underlying layout types to packages/core/logging/core-logging-server/src/layout.ts
From here, I see that I underestimated the amount of types that were nested under LoggerContextConfigInput
:
RewriteAppenderConfig
requires RewritePolicyConfig
RollingFileAppenderConfig
requires both TriggeringPolicyConfig
and RollingStrategyConfig
So,
RewritePolicyConfig
and underlying types to packages/core/logging/core-logging-server/src/appenders/rewrite.ts
RollingFileAppenderConfig
, TriggeringPolicyConfig
and underlying types to packages/core/logging/core-logging-server/src/appenders/rolling_file.ts
At this point, all the imports in all the files of the @kbn/core-logging-server
seems to be valid, so I edit the entrypoint (packages/core/logging/core-logging-server/src/index.ts
) to export all the types we moved there.
@kbn/core-logging-server
bazel config fileCommit: https://github.com/elastic/kibana/pull/134438/commits/31ea41bd7be7bb0219ea4dabdf2ab5f5490ef8e6
Now that all types are moved, I need to see which external imports are performed to add the dependencies to the bazel configuration file.
We can do this manually, or just run kbn bootstrap
and see what bazel has to say about it.
I run it, and get the following output:
info [bazel] packages/core/logging/core-logging-server/src/appenders/rolling_file.ts:9:36 - error TS2307: Cannot find module '@kbn/config-schema' or its corresponding type declarations.
info [bazel]
info [bazel] 9 import type { ByteSizeValue } from '@kbn/config-schema';
info [bazel] ~~~~~~~~~~~~~~~~~~~~
info [bazel]
info [bazel] packages/core/logging/core-logging-server/src/appenders/rolling_file.ts:10:31 - error TS2307: Cannot find module 'moment-timezone' or its corresponding type declarations.
info [bazel]
info [bazel] 10 import type { Duration } from 'moment-timezone';
info [bazel] ~~~~~~~~~~~~~~~~~
info [bazel]
info [bazel] packages/core/logging/core-logging-server/src/contracts.ts:9:28 - error TS2307: Cannot find module 'rxjs' or its corresponding type declarations.
info [bazel]
info [bazel] 9 import { Observable } from 'rxjs';
info [bazel] ~~~~~~
info [bazel]
info [bazel] packages/core/logging/core-logging-server/src/logger.ts:9:28 - error TS2307: Cannot find module '@kbn/logging' or its corresponding type declarations.
info [bazel]
info [bazel] 9 import { LogLevelId } from '@kbn/logging';
info [bazel] ~~~~~~~~~~~~~~
So I know that I need to add the dependencies to the following libraries:
moment-timezone
rxjs
@kbn/config-schema
@kbn/logging
We're only exporting types here, so I just need to add them to the TYPES_DEPS
list of packages/core/logging/core-logging-server/BUILD.bazel
I add the following entries to TYPE_DEPS
:
"@npm//@types/moment-timezone",
"@npm//rxjs",
"//packages/kbn-logging:npm_module_types",
"//packages/kbn-config-schema:npm_module_types"
then re-run yarn run kbn bootstrap
, and the bazel compilation passes.
Commit: https://github.com/elastic/kibana/pull/134438/commits/ae756db7ee6fc54c01e49677c54a6dede2535877
At this point I moved all public types into the new @kbn/core-logging-server
package, but I did not change any import/export in/from src/core/server/logging
, and I need to fix that.
There are multiple options to detect all the new problem / failures:
I usually just push the PR to GH at this point to let the CI list the issues for me.
I do it, and get the first failures batch a few minutes later: https://buildkite.com/elastic/kibana-pull-request/builds/50978#018166e5-1472-4727-890b-9b6a2a8b717c
All violations are usually not listed, as fixing things can surface other issues, so I do it iteratively: Fix, push, wait, fix, push, wait. In this PR, I amended and force-pushed to keep all the changes of this step inside a single commit for the sake of clarity, but I usually don't do that.
We know we are done when the Check
CI step fails on the generated documentation instead of the type violations:
warn You have changed the signature of the core/server public API
--
| warn To accept these changes run `node scripts/check_published_api_changes.js --accept` and then:
| 1. Commit the updated documentation and API review file '/var/lib/buildkite-agent/builds/kb-n2-2-spot-9aefc3bd6fad3096/elastic/kibana-pull-request/kibana/src/core/server/server.api.md'
| 2. Describe the change in your PR including whether it's a major, minor or patch
@kbn/core-logging-server-internal
Commit: https://github.com/elastic/kibana/pull/134438/commits/1c9bbec6f1f9182ab4f98fafbfd21672d047045c
I'm going to move all files under src/core/server/logging
to @kbn/core-logging-server-internal
, at the exception of:
integration_tests
folder. Integration tests will have to live in their own package, but we don't need to handle that nowsrc/core/server/logging/logging_service.mock.ts
src/core/server/logging/logging_system.mock.ts
README.mdx
file because I'm not 100% sure we want/can move that@kbn/core-logging-server-internal
bazel config fileCommit: https://github.com/elastic/kibana/pull/134438/commits/a5a1febb3e798b12c849537e49be72a6a7f5180a
Once moved, I need to check that the imports are all valid. But given the number of files, I don't want to do that manually, so I run yarn run kbn bootstrap
to see what Bazel has to say.
I get a lot of errors, but most of them are because we did not specify the proper dependencies in the bazel configuration file, packages/core/logging/core-logging-server-internal/BUILD.bazel
, so I start by looking at the error about unknown modules in the bazel errors to see what I have to add, E.g
info [bazel] packages/core/logging/core-logging-server-internal/src/layouts/pattern_layout.ts:10:35 - error TS2307: Cannot find module '@kbn/logging' or its corresponding type declarations.
info [bazel]
info [bazel] 10 import { LogRecord, Layout } from '@kbn/logging';
That way, I see that I'm importing from:
moment-timezone
lodash
elastic-apm-node
@elastic/safer-lodash-set
@kbn/config-schema
@kbn/logging
@kbn/core-base-server-internal
@kbn/core-logging-server
However this time, we don't only have types in our package, but also concrete code, so I will have to check which dependency can be only added to TYPES_DEPS
, and which ones have to be also added to RUNTIME_DEPS
.
I know that most external libraries are used to import concrete stuff from them, so these ones have to be set into the two dep lists. Regarding the internal packages, packages only exporting types (e.g @kbn/logging
) can only be added to TYPE_DEPS
, where packages used for concrete implementations (e.g @kbn/config-schema
) will have to be listed in both dep lists.
Commit: https://github.com/elastic/kibana/pull/134438/commits/867487e210f7d9ad665329284393fd60bb6d0357
Now that the bazel package build is passing for @kbn/core-logging-server-internal
, I want to make sure that the tests we moved here are still passing.
Worth nothing that given the test files are excluded from the bazel build, potential errors in imports coming from test files will be missed by the bazel build, so I want to make sure that I didn't forget anything here too.
I just run node scripts/jest packages/core/logging/core-logging-server-internal
, and I see that I got 2 suites failing:
FAIL packages/core/logging/core-logging-server-internal/src/layouts/pattern_layout.test.ts
● Test suite failed to run
Cannot find module '../../../test_helpers/strip_ansi_snapshot_serializer' from 'packages/core/logging/core-logging-server-internal/src/layouts/pattern_layout.test.ts'
and
FAIL packages/core/logging/core-logging-server-internal/src/logging_service.test.ts
● Test suite failed to run
Cannot find module './logging_system.mock' from 'packages/core/logging/core-logging-server-internal/src/logging_service.test.ts'
The first one is just about importing an helper from src/core/test_helpers
. Taking a quick look, this is just a 5 liner util, so I decide to just duplicate it in the single test file of the package using it (and we will later remove it from src/core/test_helpers
)
The second one is a bit more problematic: the test suite wants to import the LoggingSystem
mock for internal testing. However, these mocks will be exposed from their dedicated package, and the mock package will depend on @kbn/core-logging-server-internal
, so I can't just import the mocks for this suite.
However looking at the suite, the mock isn't expected to have any behavior. Given it's only one test suite, and given the amount of work to extract the logging system mock (and types) to other packages doesn't seems worth it, I decide to manually create a mock following the loggingSystem interface from the test suite. This is not ideal but seems like the pragmatic approach.
Once these two modifications are done, all the tests are passing.
@kbn/core-logging-server-mocks
Commit: https://github.com/elastic/kibana/pull/134438/commits/1618d54659c0e4313ac4cdb24a9fc60b6f8e4bab
In theory I should adapt the imports from src/core/server
first, but given there's only 2 mock files, I decide to move the mocks to their package first and then adapt the import for both packages.
So I move
logging_service.mock.ts
logging_system.mock.ts
From src/core/server/logging
to packages/core/logging/core-logging-server-mocks/src
I then edit the bazel configuration file to add the required dependencies. Given there's only 2 files in this package, I don't even bother running kbn bootstrap
and just check the two files to see which packages we're importing from:
@kbn/logging
@kbn/logging-mocks
@kbn/utility-types
@kbn/core-logging-server
@kbn/core-logging-server-internal
Once added, I run kbn bootstrap
to make sure that the package is correctly built.
@kbn/core-logging-server-internal
and @kbn/core-logging-server-mocks`Commit: https://github.com/elastic/kibana/pull/134438/commits/ccb5542367d7ba9a367dc4b604ad54c109a11d39
As done in 6.
, we now need to update and fix the imports to the things we moved to packages. I just pushed to CI and followed the same logic: push/wait/fix and so on.
Commit: https://github.com/elastic/kibana/pull/134438/commits/bae0451866447d2d0743363d45c2b11512fbd701
Even if we're going to get rid of it, we currently still need to update the documentation. Also note that it seems that the script sometime requires to clean kibana to function properly.
To be sure, I just run:
yarn run kbn clean && yarn run kbn bootstrap && node scripts/check_published_api_changes.js --docs --accept
Then commit the result
Commit: https://github.com/elastic/kibana/pull/134438/commits/6a7a6f115ad62035285a9ad9f87fb3e0e5b5174c
Just remove the auto-generated text and add some very basic description to the README.md
file of the 3 newly created packages.
Now that all the publicly accessible types are exported from @kbn/core-logging-server
, we can see that a lot of them are missing proper ts documentation, so as a best-effort, I add some
Part of https://github.com/elastic/kibana/issues/134112
No internal core dependencies
Import some snapshot serializer from core/test_helpers
May want to move ILoggingSystem to
@kbn/logging
New packages:
@kbn/core-logging-server
@kbn/core-logging-server-internal
@kbn/core-logging-server-mocks