-
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
Remove executionLog to reduce memory consumption #111911
Conversation
@elasticmachine merge upstream |
await cleanup(client, executionLog, finalState); | ||
dumpExecutionLog(logger, logMessagePrefix, executionLog); |
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.
Looking at the code, it seems to be the case, but just to be sure:
-
When the migration logger's level is
DEBUG
, we're still outputting the same information we previously stored in the executionLogs, right? -
In case of failure, the full cause is still outputted in the logs even when not in
DEBUG
level?
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.
- Yes
- We still output the full fatal message + traces etc. The difference is, before we would also log the execution log including the full state and responses to each action. Because this was a
meta
field, it wasn't visible by default with the legacy logger config, you would have to switch to NP JSON logging to actually see this. So in practice for most on-prem and cloud users we won't loose any details.
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 missed that we only logged the state transition meta information from the executionLog. So I'm adding debug logging when there's a state transition so that we can inspect the complete state after every step when debugging is enabled.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
const outputStream = this.outputStream; | ||
this.outputStream = undefined; | ||
|
||
outputStream.end(() => { |
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.
Can't we just
this.outputStream.end(() => {
resolve();
});
this.outputStream = undefined;
?
I think the handler will never be executed synchronously?
{ | ||
kibana: { | ||
migrations: { | ||
state: currState, | ||
duration: tookMs, | ||
}, | ||
}, | ||
} |
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.
This will only be visible when using the JSON
layout (in addition to the debug level I mean), but I guess this can't be helped.
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Rudolf Meijering <[email protected]>
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
…12223) * Remove executionLog to reduce memory consumption (#111911) Co-authored-by: Kibana Machine <[email protected]> * Fix tests Co-authored-by: Rudolf Meijering <[email protected]>
Summary
Saves at least 400MB of heap when doing a migration of 100k saved objects (because of GC it's hard to measure exactly, but while this migration previously required
--max_old_space_size=1000
it now passes with--max_old_space_size=600
)When I wrote the initial implementation I was quite worried we'd get some weird failure edge case that we can't reproduce. So I added the executionLog so that if a migration fails we could log a lot of detail (i.e. the complete state and all action responses). However, we haven't really needed these detailed logs and we can still get this information by asking users to run migrations with debug logging enabled. So at the expense of losing some detail in our logs migrations have a much better chance of succeeding when there's a huge amount of saved objects, or many large saved objects.
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers