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

Add sidekiq config propagate_traces to control trace header injection #2588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sl0thentr0py
Copy link
Member

closes #2531
part of #2476

@sl0thentr0py sl0thentr0py force-pushed the neel/sidekiq-propagate-traces branch from c38d916 to 02a5e26 Compare March 19, 2025 13:57
@sl0thentr0py sl0thentr0py requested a review from solnic March 19, 2025 13:58
@sl0thentr0py sl0thentr0py changed the title Add sidekiq config propagate_traces to control trace header injection Add sidekiq config propagate_traces to control trace header injection Mar 19, 2025
it "has correct default value" do
expect(subject.report_only_dead_jobs).to eq(false)
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unrelated?

Copy link
Member Author

Choose a reason for hiding this comment

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

was missing so just added it

Copy link
Collaborator

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Left a question


if Sentry.configuration.sidekiq.propagate_traces
job["trace_propagation_headers"] ||= Sentry.get_trace_propagation_headers
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this a breaking change in a way? Previously it would do it unconditionally. Now you have to explicitly enable it. I understand that the original behavior was problematic though so maybe it's fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

it defaults to true

@sl0thentr0py sl0thentr0py requested a review from solnic March 20, 2025 12:31
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.

Unrelated Sidekiq job runs being grouped in same trace
2 participants