-
Notifications
You must be signed in to change notification settings - Fork 26
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
Improve error messages in garm log #314
base: main
Are you sure you want to change the base?
Conversation
8230b40
to
8a31d81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I should have added more context to those messages.
database/sql/jobs.go
Outdated
@@ -244,7 +246,8 @@ func (s *sqlDatabase) CreateOrUpdateJob(ctx context.Context, job params.Job) (pa | |||
if err == nil { | |||
workflowJob.InstanceID = &instance.ID | |||
} else { | |||
slog.With(slog.Any("error", err)).ErrorContext(ctx, "failed to get instance by name") | |||
// This usually is very normal as not all jobs run on our runners. | |||
slog.DebugContext(ctx, fmt.Sprintf("failed to get instance by name: %s", job.RunnerName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above. The level change to DebugContext
is fine.
Co-authored-by: Gabriel <[email protected]>
8255c5f
to
d6de596
Compare
I think we need to bump |
finding errors in logs is very important and there are some logs that are
errors
but seem to be rather normal:our
runners. This always happens in a scenario where our garm runners are not the only runner setup (and we still get the workflow_job events, e.g. enterprise wide hooks)