Skip to content

fix context propagation for logs#1118

Merged
giortzisg merged 3 commits intomasterfrom
fix-slog-ctx
Oct 28, 2025
Merged

fix context propagation for logs#1118
giortzisg merged 3 commits intomasterfrom
fix-slog-ctx

Conversation

@giortzisg
Copy link
Contributor

@giortzisg giortzisg commented Oct 24, 2025

Description

This PR fixes two issues with how the context was passed on logs:

  • check for falling back on the initialization context, if slog is passing context.Background()
  • use SpanFromContext first and then default back to GetHubFromContext.

Issues

@giortzisg giortzisg self-assigned this Oct 24, 2025
@giortzisg giortzisg added the Bug Issue type label Oct 24, 2025
cursor[bot]

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Oct 24, 2025

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.89%. Comparing base (c982e6f) to head (1ad35f2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
slog/sentryslog.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   86.67%   86.89%   +0.22%     
==========================================
  Files          60       61       +1     
  Lines        6964     5557    -1407     
==========================================
- Hits         6036     4829    -1207     
+ Misses        743      542     -201     
- Partials      185      186       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


func (h *logHandler) Handle(ctx context.Context, record slog.Record) error {
// when logging without context, slog passes `context.Background`. Check for span existence to not overwrite the root context.
if sentry.SpanFromContext(ctx) == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

StartSpan adds the span as a context key, so it doesn't matter where it originates from, it only matters if the key exists.

Copy link
Member

@lcian lcian left a comment

Choose a reason for hiding this comment

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

Should we add tests for the change?
Does any other logger need to be updated with this fix?

@linear
Copy link

linear bot commented Oct 28, 2025

Copy link
Member

@lcian lcian left a comment

Choose a reason for hiding this comment

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

LGTM

@giortzisg giortzisg merged commit 97e65bf into master Oct 28, 2025
17 of 18 checks passed
@giortzisg giortzisg deleted the fix-slog-ctx branch October 28, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Issue type

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Slog integration only attaches to trace when using slog.***Context()

2 participants