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

Fix: log more information about Chromium's run #46

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

mem
Copy link
Contributor

@mem mem commented Nov 1, 2024

Regardless of whether there was an error or not, log some information about the run (resources, exit code, etc).

@mem mem requested a review from a team as a code owner November 1, 2024 23:06
crocochrome.go Outdated Show resolved Hide resolved
@roobre
Copy link
Member

roobre commented Nov 4, 2024

Renovate is now fixed and alpine updated, so Build container image should pass on the next rebase.

crocochrome.go Outdated
Comment on lines 334 to 335
logger.LogAttrs(ctx, slog.LevelError, "chromium process finished", attrs...)
logger.Error("chromium output", "stdout", stdout.String(), "stderr", stderr.String())
Copy link
Member

Choose a reason for hiding this comment

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

Should we merge these two log lines? They were originally separated out of lazyness rather than purpose, and now that we're dynamically creating the message we could as well incorporate stdout/stderr in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I'm a little skeptical about this, because there's a limit to the length of lines that will be pushed to Loki (256 kB).

I expect that if there's an error that triggers this, the useful information will be at the end of the log, which might get truncated.

But let's give this a try...

@roobre
Copy link
Member

roobre commented Nov 4, 2024

#51 caused a conflict but should be pretty minor.

@mem mem force-pushed the mem/log-more-information branch from 9c20d0e to 08ecfa3 Compare November 4, 2024 15:00
Regardless of whether there was an error or not, log some information
about the run (resources, exit code, etc).

Signed-off-by: Marcelo E. Magallon <[email protected]>
@mem mem force-pushed the mem/log-more-information branch from 08ecfa3 to 885916e Compare November 4, 2024 17:42
@mem mem requested a review from roobre November 4, 2024 17:53
@roobre
Copy link
Member

roobre commented Nov 4, 2024

Looking great!

@roobre roobre merged commit cf5d9be into main Nov 5, 2024
4 checks passed
@roobre roobre deleted the mem/log-more-information branch November 5, 2024 09:49
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.

2 participants