Skip capture of TRACE workunits by default
#13483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stuhood
commented
Nov 4, 2021
|
Commits are useful to review independently. |
kaos
reviewed
Nov 4, 2021
There was a problem hiding this comment.
Cool, thanks!
I'm trying to understand how this interacts with --log-levels-by-target? It'd be helpful to confirm that can still result in workunits being created at TRACE even if everything else is DEBUG - probably worth a unit test. Also might be worth updating the help message.
If you set --streaming-workunits-level=INFO, I don't see code to filter out DEBUG? The max_level would be set to at least DEBUG because of the dynamic_ui, so those workunits will be uploaded. We need to filter them out though before sending to streaming workunit handler.
|
@stuhood please hold off on landing this until we get a chance to see if this causes some issues with the toolchain backend stuff that needs to be addressed. (I didn't get a chance to look at this yet) |
This happens via |
stuhood
force-pushed
the
stuhood/configurable-workunit-level
branch
from
8dac301
to
5afa07f
Compare
stuhood
commented
Feb 19, 2022
stuhood
added a commit
that referenced
this pull request
Feb 21, 2022
In order to unblock optionally recording workunits without losing counter accuracy (in #13483), move to recording counters globally via the `StreamingWorkunitContext`. [ci skip-build-wheels]
alonsodomin
pushed a commit
to alonsodomin/pants
that referenced
this pull request
Feb 25, 2022
…#14541) In order to unblock optionally recording workunits without losing counter accuracy (in pantsbuild#13483), move to recording counters globally via the `StreamingWorkunitContext`. [ci skip-build-wheels]
… default (`DEBUG`) than before (`TRACE`). [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…ults in skipping the construction of `TRACE` workunits. [ci skip-build-wheels]
[ci skip-build-wheels] [ci skip-rust]
|
Anecdotally: this reduces memory usage for local runs of |
stuhood
force-pushed
the
stuhood/configurable-workunit-level
branch
from
5afa07f
to
0adcef7
Compare
[ci skip-rust] [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
stuhood
force-pushed
the
stuhood/configurable-workunit-level
branch
from
0adcef7
to
8adecea
Compare
Eric-Arellano
approved these changes
Feb 28, 2022
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Mar 3, 2022
…being filtered out, which caused their (blocked) parents to render.
This was referenced Mar 14, 2022
stuhood
added a commit
that referenced
this pull request
Mar 29, 2022
…e themselves (#14934) #13483 broke the rendering of `EngineAwareReturnType` implementations which relied on starting a workunit at `Level::TRACE`, and then escalating its level to something visible (usually `INFO` or greater) when it completed. `check` outputs for the JVM were strongly affected (see #14867), since they relied on the fact that `FallibleClasspathEntry` escalates to `ERROR` to render compile errors. To resolve this, we roll back a portion of #13483. Rather than not recording the workunit at all, we instead record it in the `WorkunitStore` as "disabled", which is signaled by a workunit not having any `WorkunitMetadata`. This has some of the efficiency benefits of #13483 (because we continue to skip heap allocating the metadata's fields), but allows a workunit to escalate itself from disabled to enabled as it completes by specifying a non-disabled level in `RunningWorkunit::update_metadata`. The recording of "disabled" workunits is additionally necessary for #14680, because otherwise workunits which were not actually recorded would break the tracking of multiple parents: when adding a new parent to a workunit, you need an existing `SpanId` corresponding to the work that you are adding a parent to (or else you might accidentally depend on a parent arbitrarily far up the stack). Fixes #14867.
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Mar 29, 2022
…e themselves (pantsbuild#14934) pantsbuild#13483 broke the rendering of `EngineAwareReturnType` implementations which relied on starting a workunit at `Level::TRACE`, and then escalating its level to something visible (usually `INFO` or greater) when it completed. `check` outputs for the JVM were strongly affected (see pantsbuild#14867), since they relied on the fact that `FallibleClasspathEntry` escalates to `ERROR` to render compile errors. To resolve this, we roll back a portion of pantsbuild#13483. Rather than not recording the workunit at all, we instead record it in the `WorkunitStore` as "disabled", which is signaled by a workunit not having any `WorkunitMetadata`. This has some of the efficiency benefits of pantsbuild#13483 (because we continue to skip heap allocating the metadata's fields), but allows a workunit to escalate itself from disabled to enabled as it completes by specifying a non-disabled level in `RunningWorkunit::update_metadata`. The recording of "disabled" workunits is additionally necessary for pantsbuild#14680, because otherwise workunits which were not actually recorded would break the tracking of multiple parents: when adding a new parent to a workunit, you need an existing `SpanId` corresponding to the work that you are adding a parent to (or else you might accidentally depend on a parent arbitrarily far up the stack). Fixes pantsbuild#14867. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
stuhood
added a commit
that referenced
this pull request
Mar 29, 2022
…e themselves (cherrypick of #14854, #14856, #14934) (#14942) Fix missing `check` output by allowing disabled workunits to re-enable themselves (#14934) #13483 broke the rendering of `EngineAwareReturnType` implementations which relied on starting a workunit at `Level::TRACE`, and then escalating its level to something visible (usually `INFO` or greater) when it completed. `check` outputs for the JVM were strongly affected (see #14867), since they relied on the fact that `FallibleClasspathEntry` escalates to `ERROR` to render compile errors. To resolve this, we roll back a portion of #13483. Rather than not recording the workunit at all, we instead record it in the `WorkunitStore` as "disabled", which is signaled by a workunit not having any `WorkunitMetadata`. This has some of the efficiency benefits of #13483 (because we continue to skip heap allocating the metadata's fields), but allows a workunit to escalate itself from disabled to enabled as it completes by specifying a non-disabled level in `RunningWorkunit::update_metadata`. The recording of "disabled" workunits is additionally necessary for #14680, because otherwise workunits which were not actually recorded would break the tracking of multiple parents: when adding a new parent to a workunit, you need an existing `SpanId` corresponding to the work that you are adding a parent to (or else you might accidentally depend on a parent arbitrarily far up the stack). Fixes #14867. [ci skip-build-wheels]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, workunits are captured and stored (in memory, but also via any
StreamingWorkunitHandlercallbacks), regardless of their level. But in many cases, this results in tens of thousands of workunits, only a fraction of which will ever be consumed.For some runs, the ratio between
TRACEworkunits and all other workunits (DEBUG, etc) is16to1... meaning that disablingTRACEby default results in significantly less data being captured (and less CPU usage as well).In order to continue to support capturing finer grained details, the
WorkunitStoreis adjusted to capture only what is required by:--dynamic-ui, if it is enabled--streaming-workunits-leveloption--leveloption--log-levels-by-targetvaluesMicrobenchmarks show about a 5% improvement (for
hyperfine -w 2 './pants test --force --no-pytest-timeouts src/python/pants/engine/internals/engine_benchmarks_test.py'), and an 11-15% reduction in memory usage for./pants dependencies --transitive ::.[ci skip-build-wheels]