Skip to content
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

feat: added support for streaming stdout when --progress=plain #7069

Merged
merged 6 commits into from
Apr 10, 2024

Conversation

morlay
Copy link
Contributor

@morlay morlay commented Apr 10, 2024

fix #7057

@morlay
Copy link
Contributor Author

morlay commented Apr 10, 2024

image

logs in real CI. (no idea about the color lost issue)

return nil
}

func (t *streamingExporter) getSpanName(spanID trace.SpanID) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the span name is not always get cause otel emit the spans until span.End() called.
may consider set span name in log attrs too.

@morlay morlay force-pushed the streaming-output branch 2 times, most recently from b8b2b38 to 74f8714 Compare April 10, 2024 04:59
@jedevc
Copy link
Member

jedevc commented Apr 10, 2024

I think this makes sense 🎉 I'm really not familiar with how frontend.go works though.

To me, it feels confusing that the logic for adding new views/etc is really complex - ideally, we'd have an interface that a new view would be able to implement. Then the fancy new progress view and the plain one would all just be implementations of that interface.

@jedevc jedevc requested a review from vito April 10, 2024 10:57
@jedevc
Copy link
Member

jedevc commented Apr 10, 2024

There were still some color issues (particularly in the summary that's still printed at the end), so pushed a fix for that (the extra WithTTY calls can be removed once vito/progrock#14 is merged and updated).

@jedevc
Copy link
Member

jedevc commented Apr 10, 2024

I'd like to do some further refactoring of this later, but for now, this fixes the issue, and it would be awesome to include this in v0.11.1, so going to approve now, and I'll happily iterate on this after the release.

@jedevc jedevc added this to the v0.11.1 milestone Apr 10, 2024
morlay and others added 3 commits April 10, 2024 14:07
fix dagger#7057

Signed-off-by: Morlay <morlay.null@gmail.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
func (w *batchPrinter) render(out *termenv.Output, final bool) {
w.mu.Lock()
defer w.mu.Unlock()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedevc

it could ignore logs without spanName for now.

spanName := w.getSpanName(w.spanID)
if spanName == "" {
    return
}

@morlay
Copy link
Contributor Author

morlay commented Apr 10, 2024

@jedevc spanName may empty.

image

it could safe fixed by https://github.com/dagger/dagger/pull/7069/files#r1559461303

This is needed for printing out the final data!

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
Copy link
Member

jedevc commented Apr 10, 2024

@morlay can you share an example of where span is empty?

@morlay
Copy link
Contributor Author

morlay commented Apr 10, 2024

@morlay can you share an example of where span is empty?

this first 6: in that image.

It logs before trace span emitted.

@jedevc
Copy link
Member

jedevc commented Apr 10, 2024

Right, yeah, I meant, do you have a publicly-available example I can run to get this?

I'm trying to understand why this can happen.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@morlay
Copy link
Contributor Author

morlay commented Apr 10, 2024

Right, yeah, I meant, do you have a publicly-available example I can run to get this?

I'm trying to understand why this can happen.

It from github actions logs of my tool.
https://github.com/octohelm/piper

It base dagger for container.
But could execute in local/remote hosts.
Like some build system or the Ansible did.

To make logs in same view, I reuse idtui when dagger v0.11.0 released.

So in local host execution, it will be fast.
Then got the issue. 😂

All tasks defined by cue lang.
https://github.com/octohelm/piper/blob/main/piper.cue

It could be run TTY=0 make build with this project

To see the issue should delete this condition
https://github.com/octohelm/piper/blob/main/internal/logger/steaming_exporter.go#L152

@jedevc jedevc linked an issue Apr 10, 2024 that may be closed by this pull request
@morlay
Copy link
Contributor Author

morlay commented Apr 10, 2024

https://github.com/dagger/dagger/blob/main/dagql/idtui/types.go#L133

Btw, in my case. It will make my final report hide some spans. 😂

Could we make it configable?

@vito
Copy link
Contributor

vito commented Apr 10, 2024

To me, it feels confusing that the logic for adding new views/etc is really complex - ideally, we'd have an interface that a new view would be able to implement. Then the fancy new progress view and the plain one would all just be implementations of that interface.

This view is fundamentally different so it makes sense to me. Normally we're rendering into a viewport and dynamically redrawing parts of the screen, TUI style. This one just needs to print output directly without any funny business and let it go into the scrollback infinitely, skipping past all those moving parts and disregarding screen size and all that.

In the end they're both implementing an interface, just perhaps a lower level one than you might have expected (OpenTelemetry span/log exporter).

@vito
Copy link
Contributor

vito commented Apr 10, 2024

Could we make it configable?

Already is - if you pass -vvv (verbosity level 3) it'll show everything. Probably need that in our docs. :)

@morlay
Copy link
Contributor Author

morlay commented Apr 10, 2024

Already is - if you pass -vvv (verbosity level 3) it'll show everything. Probably need that in our docs. :)

but with it, the final reporter is too noisy. 😂
May some option could define the should show.

Copy link
Contributor

@vito vito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start, but I noticed it seems impossible to interrupt it at the moment with Ctrl+C. But, I think that was already broken in plain mode, so approving anyway; this is still a net positive. I think we can merge this as-is and I can try to fix that + maybe do some refactoring after. Thanks a bunch!

@jedevc jedevc merged commit 2bc90a5 into dagger:main Apr 10, 2024
42 of 43 checks passed
@vito
Copy link
Contributor

vito commented Apr 10, 2024

but with it, the final reporter is too noisy. 😂
May some option could define the should show.

I'd prefer to avoid a bunch of flags, but feel free to make it a param or var instead of a const, if you're using this code directly.

vikram-dagger pushed a commit to vikram-dagger/dagger that referenced this pull request May 3, 2024
…r#7069)

* feat: added support for streaming stdout when --progress=plain

fix dagger#7057

Signed-off-by: Morlay <morlay.null@gmail.com>

* chore: fix some linting issues

Signed-off-by: Justin Chadwell <me@jedevc.com>

* fix: ensure we set tty everywhere

Signed-off-by: Justin Chadwell <me@jedevc.com>

* streaming output should still collect db

This is needed for printing out the final data!

Signed-off-by: Justin Chadwell <me@jedevc.com>

* apply suggested fix for empty span name

Signed-off-by: Justin Chadwell <me@jedevc.com>

* import newoutput from progrock

Signed-off-by: Justin Chadwell <me@jedevc.com>

---------

Signed-off-by: Morlay <morlay.null@gmail.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Co-authored-by: Justin Chadwell <me@jedevc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants