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(runners): increase the log level to WARN when using the enable_job_queued_check parameter #3046

Merged
merged 4 commits into from
Mar 17, 2023

Conversation

Fgerthoffert
Copy link
Contributor

@Fgerthoffert Fgerthoffert commented Mar 10, 2023

The readme section detailing the enable_job_queued_check makes it an appealing function, but it was unclear to me that it could cause some issues when used with ephemeral runners and matrix workflows.

It is particularly challenging to notice for organizations with a very large number of repositories, at times with low activity the issue is not present, but when matrix jobs are started or when multiple repositories are triggering jobs around the same time window (such as cron'd workflow), this issue is more likely to be faced.

For example, on the execution that allowed me to find the cause, when starting a matrix of 9 jobs, 4 of them were actually triggering this log message:

2023-03-10 15:29:09.728  INFO  [scale-up:a0502c68-f9b9-5d2d-afd6-70edb7486316 index.js:135249  isJobQueued] Job not queued 

This log message, aside from being one amongst many other INFO messages, does not provide many details about the consequences.

I suggest bumping the level to WARN and adding more details in the message itself.

Related discussion: #2931

@npalm npalm self-requested a review March 10, 2023 17:49
@Fgerthoffert Fgerthoffert changed the title Increased the log level to WARN when using the enable_job_queued_check parameter docs: Increase the log level to WARN when using the enable_job_queued_check parameter Mar 11, 2023
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

LGTM

@npalm
Copy link
Member

npalm commented Mar 15, 2023

I think the logging messages we have can be improved. Thanks for your contribution. I have also created a PR to replace the current framework (which is out-dated). The replacement will not direct change the log messages. But provides with each messages the context. And we drop support for line based logging, moving to json which is much easier to parse. See #3037

@npalm npalm changed the title docs: Increase the log level to WARN when using the enable_job_queued_check parameter fix(runners): increase the log level to WARN when using the enable_job_queued_check parameter Mar 15, 2023
@npalm npalm merged commit 1de73bf into philips-labs:main Mar 17, 2023
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