Skip to content

Listener stdout logging#2291

Merged
fhammerl merged 15 commits into
mainfrom
avastancu/fhammerl/stdout-logging
Dec 6, 2022
Merged

Listener stdout logging#2291
fhammerl merged 15 commits into
mainfrom
avastancu/fhammerl/stdout-logging

Conversation

@AvaStancu

@AvaStancu AvaStancu commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

Solves the Listener part needed for https://github.com/github/c2c-actions-runtime/issues/2065
Worker part to follow

image

@AvaStancu AvaStancu requested a review from a team as a code owner December 1, 2022 17:05
Comment thread src/Runner.Common/HostTraceListener.cs Outdated
@AvaStancu AvaStancu force-pushed the avastancu/fhammerl/stdout-logging branch from 011bc87 to e02f9ab Compare December 2, 2022 09:15
Comment thread src/Runner.Common/Terminal.cs
Comment thread src/Runner.Common/StdoutTraceListener.cs Outdated
Comment thread src/Runner.Common/Terminal.cs
Comment thread src/Runner.Common/Tracing.cs Outdated
Comment thread src/Runner.Common/Terminal.cs Outdated
Comment thread src/Runner.Common/HostContext.cs Outdated
Comment thread src/Runner.Common/StdoutTraceListener.cs Outdated
Comment thread src/Runner.Common/Terminal.cs Outdated
Console.WriteLine($"# {message}");
Console.WriteLine();
}
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need to update all the Write*() functions in the file to have the else?

@TingluoHuang TingluoHuang Dec 5, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be weird to change those errors get printed to STDOUT instead STDERR.

@fhammerl fhammerl Dec 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need to update all the Write*() functions in the file to have the else?

only the Write*() that don't have a Trace.* Instead of an else, should we actually Trace these always?

We could also do Trace.Info($"WRITE SECTION # {message}"); and Trace.Info($"WRITE SUCCESS √ {message}"); to keep it consistent with other traces in the other Write*() functions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was more focused on the WriteError() parts. 😄

@fhammerl fhammerl Dec 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

might be weird to change those errors get printed to STDOUT instead STDERR.

Good point 🤔
In StdoutTraceListener, we could use Console.Error.WriteLine(message); if TraceEventType is ERROR (or CRITICAL)

I think kubectl logs [pod] gives a combined stdout/stderr stream, so it should work for us

@fhammerl fhammerl force-pushed the avastancu/fhammerl/stdout-logging branch from 8e09417 to 3e14982 Compare December 6, 2022 13:50
@fhammerl fhammerl merged commit 088981a into main Dec 6, 2022
@fhammerl fhammerl deleted the avastancu/fhammerl/stdout-logging branch December 6, 2022 15:16
nikola-jokic pushed a commit to nikola-jokic/runner that referenced this pull request May 12, 2023
* Added env variable to control wether the terminal is silent

* Log to stdout if PrintLogToStdout is enabled

* Extracted console logging to stdouttracelistener

* Remove useless usings

* Rewrite TraceListener as superclass

* Only print to stdout if env is set

* Add comment for Console.Out

* Format Listener

* Revert var name in terminal

* Check env in hostcontext instead of Tracing constructor

* Remove superclass & dupe logging code

* Log hostType

* Readonly '_' prefix 'hostType'

* Fix test

* Revert Terminal change

Co-authored-by: Ferenc Hammerl <31069338+fhammerl@users.noreply.github.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

Development

Successfully merging this pull request may close these issues.

3 participants