-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix SO migration integration tests #98478
Conversation
enforce buffer usage for all the logs created during disposal phase
Pinging @elastic/kibana-core (Team:Core) |
@@ -28,6 +28,7 @@ export function getServerWatchPaths({ pluginPaths, pluginScanDirs }: Options) { | |||
(acc: string[], path) => [ | |||
...acc, | |||
Path.resolve(path, 'test/**'), | |||
Path.resolve(path, 'integration_tests/**'), |
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.
noticed that watcher restarts Kibana server when our integration tests write to a log file.
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.
LGTM (post snapshot update)
// reconfigure all the loggers without configuration to have them use the buffer | ||
// appender while we are awaiting for the appenders to be disposed. |
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.
NIT: should we keep the comments to understand why we are setting the loggers twice?
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 left it https://github.com/elastic/kibana/pull/98478/files#diff-92a0cd1f9422606d47a5c270e10021d1970ef35ab885406286c0de6e6237ec0cR214
Or you mean something else?
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.
right! sorry I missed it :)
for (const [loggerKey, loggerAdapter] of this.loggers) { | ||
loggerAdapter.updateLogger(this.createLogger(loggerKey, config)); | ||
} | ||
this.computedConfig = config; |
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.
NIT: I wonder if this logic could be extracted to a common method where config
can be undefined
. Then, applyBaseConfig
has the business logic that decides when to call it with undefined
and when with a provisioned config. Does it make sense?
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.
Maybe, but I find both method names to be more self-descriptive. Also, we might need different logic in the future, so not sure we should strive for DRY in this case.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/tls·ts.apis SecuritySolution Endpoints Tls Test with Packetbeat Tls Overview Test Ensure data is returned for FlowTarget.DestinationStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
* do not restart Kibana server on integration tests writing logs * unskip tests * do not write to ended stream to avoid a race condition * revert changes to the File appender * fix race condition on the logging_system level enforce buffer usage for all the logs created during disposal phase # Conflicts: # src/core/server/saved_objects/migrationsv2/integration_tests/rewriting_id.test.ts
* do not restart Kibana server on integration tests writing logs * unskip tests * do not write to ended stream to avoid a race condition * revert changes to the File appender * fix race condition on the logging_system level enforce buffer usage for all the logs created during disposal phase # Conflicts: # src/core/server/saved_objects/migrationsv2/integration_tests/rewriting_id.test.ts
Summary
Closes #98351
Closes #98352
There is a race condition when
NotReady
server writes logs to thefile
appender duringdispose
lifecycle.We already enforce all the existing appenders to write the BufferAppender while old appenders are being disposed.
This PR adds this logic for newly created appenders as well.