tui: associate effects to their origin, not their unlazier #7686
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
vito
force-pushed
the
accounting
branch
2 times, most recently
from
9e10362
to
13637d4
Compare
vito
force-pushed
the
accounting
branch
9 times, most recently
from
48cfed1
to
f087511
Compare
Signed-off-by: Alex Suraci <alex@dagger.io>
otherwise going 'out' really jumps across the universe Signed-off-by: Alex Suraci <alex@dagger.io>
Signed-off-by: Alex Suraci <alex@dagger.io>
Signed-off-by: Alex Suraci <alex@dagger.io>
Signed-off-by: Alex Suraci <alex@dagger.io>
otherwise we mark too many spans 'running'; we only show it in one place, so make the data represent that Signed-off-by: Alex Suraci <alex@dagger.io>
fixes remaining tests, addresses comment (kinda; with a helper) Signed-off-by: Alex Suraci <alex@dagger.io>
this was added while flailing on effects, now that we track running effects in IsRunning() i don't think we need this Signed-off-by: Alex Suraci <alex@dagger.io>
Signed-off-by: Alex Suraci <alex@dagger.io>
2 tasks
jedevc
reviewed
Jun 24, 2024
Comment on lines
+61
to
+64
| type buildkitSpan struct { | ||
| trace.Span | ||
| provider *buildkitTraceProvider | ||
| } |
There was a problem hiding this comment.
Aha, this is so much neater now that this logic isn't done entirely in Start 🎉
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.
The place where something becomes un-lazied is not that useful. Better to show effects at their point of origin instead. This is a bit of ✨ movie magic ✨ but it's a lot more useful than telling "the truth" (arguably an implementation detail of our telemetry plumbing).
This also effectively works around a Buildkit bug that ends up associating LLB to incorrect spans1, which would sometimes put a failed span beneath a succeeding one, making it especially hard to find.
vertexattribute, but we have the power to translate that to something likedagger.io/effect.id.vertex->dagger.io/effect.iddagger.io/llb.digests->dagger.io/effect.idsFootnotes
possibly from here which the race detector is also unhappy with) ↩