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

Jobs created by Sidekiq-cron shouldn’t propagate trace header #2391

Closed
frederikspang opened this issue Sep 1, 2024 · 6 comments · Fixed by #2446
Closed

Jobs created by Sidekiq-cron shouldn’t propagate trace header #2391

frederikspang opened this issue Sep 1, 2024 · 6 comments · Fixed by #2446

Comments

@frederikspang
Copy link
Contributor

Issue Description

Currently when sidekiq-cron enqueues jobs, trace headers are added, leaving a Long running trace with several unrelated jobs, and no root transaction.

This should be patched in Sentry-sidekiq to reset the trace id for sidekiq cron when sidekiq-cron patch is enabled

Reproduction Steps

Run app
Enqueue job every x minute
Find trace (or follow from an error in Issues tab)
See duration is several hours and without root transaction.

Expected Behavior

Each enqueued job from sidekiq cron is unrelated and with different trace IDs.

Actual Behavior

All enqueued jobs (per process/sidekiqCron Thread) are with same trace id.

Ruby Version

3.3.4

SDK Version

master

Integration and Its Version

Sidekiq

Sentry Config

No response

@sl0thentr0py
Copy link
Member

true, this is also how we do it in celery beat, will fix!

@frederikspang
Copy link
Contributor Author

Could it be something as simple as:

def enque!(*)
  # make sure the current thread has a clean hub
  Sentry.clone_hub_to_current_thread

  Sentry.with_scope do |scope|
    Sentry.with_session_tracking do
      super
    end
  end
end

Obviously, I don't know how it's done in Celery - But the one above is inspired by the Rack middleware - Except the patching of enque! method from Sidekiq cron.

@grk
Copy link

grk commented Oct 15, 2024

There's a similar issue with clockwork - everything scheduled from the clockwork process inherits one huge trace. Ideally there should be an api to do something like

Sentry.without_trace_propagation do
  SomeJob.perform_async
end

@frederikspang
Copy link
Contributor Author

Giving this a go currently.

There is a slight dependency on #2403

If I get this right, there should be a root transaction, that is the enqueuing from sidekiq-cron in one thread; That should probably be queue.publish, for this.

Would you be OK with a partial implementation, that depends on #2403 before merging? @sl0thentr0py @solnic

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 17, 2024
@sl0thentr0py
Copy link
Member

hey, I will ask @solnic to follow up here once he's back next week

@solnic
Copy link
Collaborator

solnic commented Oct 28, 2024

Would you be OK with a partial implementation, that depends on #2403 before merging? @sl0thentr0py @solnic

@frederikspang yes thank you, just open a PR and we can take it from there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants