-
Notifications
You must be signed in to change notification settings - Fork 38
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: new job type for sync canary jobs #1370
Conversation
37434cc
to
0df21fe
Compare
bbab9a3
to
0e945ef
Compare
0e945ef
to
e7d59ff
Compare
if runner.IsCanaryIgnored(&job.Canary.ObjectMeta) { | ||
return | ||
func (j CanaryJob) Run(ctx dutyjob.JobRuntime) error { | ||
ctx.GetSpan().SetAttributes(attribute.String("canary-id", j.DBCanary.ID.String())) |
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.
I don't think we should be recording traces for canary runs - they are traced internally using check_status, not sure what pushing them to opentelemetry would add ? A slow canary is more likely due to a slow remote server than a slow canary checker db ?
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.
Did it for general observability, like record all transactions to have complete overview and have a general idea of how much time the checks run is taking compared to the other database calls in the job
pkg/db/canary.go
Outdated
@@ -278,7 +279,7 @@ func FindCheck(canary pkg.Canary, name string) (*pkg.Check, error) { | |||
return &model, nil | |||
} | |||
|
|||
func FindDeletedChecksSince(ctx context.Context, since time.Time) ([]string, error) { | |||
func FindDeletedChecksSince(ctx gocontext.Context, since time.Time) ([]string, error) { |
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.
Maybe use duty's context for this as well
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.
Done 👍🏽
No description provided.