Skip to content

fix: provide init ctx to eventHandler#1133

Merged
giortzisg merged 2 commits intomasterfrom
fix/slog-eventhandler
Nov 13, 2025
Merged

fix: provide init ctx to eventHandler#1133
giortzisg merged 2 commits intomasterfrom
fix/slog-eventhandler

Conversation

@giortzisg
Copy link
Contributor

Description

Following up to #1118, we also need to fix the context propagation for the slog eventHandler, in cases that slog is used without context.

Issues

@giortzisg giortzisg requested a review from lcian November 13, 2025 12:19
@giortzisg giortzisg self-assigned this Nov 13, 2025
@linear
Copy link

linear bot commented Nov 13, 2025

@giortzisg giortzisg force-pushed the fix/slog-eventhandler branch from 4ea80e4 to 478a19a Compare November 13, 2025 12:19
@giortzisg giortzisg changed the title fix: check ptr value instead of whole error for visited chains fix: provide init ctx to eventHandler Nov 13, 2025
Comment on lines +168 to 169
ctx context.Context
option Option
Copy link

Choose a reason for hiding this comment

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

Bug: The ctx field is not propagated in eventHandler.WithAttrs() and eventHandler.WithGroup(), leading to loss of context in derived handlers.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The ctx field, newly added to the eventHandler struct, is not propagated when new handlers are created via WithAttrs() or WithGroup(). When these methods are called, the derived eventHandler instances will have ctx initialized to its zero value (nil). Consequently, calls to sentry.GetHubFromContext(h.ctx) within the Handle() method will receive a nil context, causing the system to incorrectly fall back to sentry.CurrentHub() instead of using the intended context from the original handler. This defeats the purpose of the context propagation fix introduced by this PR.

💡 Suggested Fix

Ensure the ctx field is explicitly copied from the original handler to the new eventHandler instance within both the WithAttrs() and WithGroup() methods. For example, add ctx: h.ctx, to the return struct literal in both methods.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: slog/sentryslog.go#L168-L169

Potential issue: The `ctx` field, newly added to the `eventHandler` struct, is not
propagated when new handlers are created via `WithAttrs()` or `WithGroup()`. When these
methods are called, the derived `eventHandler` instances will have `ctx` initialized to
its zero value (`nil`). Consequently, calls to `sentry.GetHubFromContext(h.ctx)` within
the `Handle()` method will receive a `nil` context, causing the system to incorrectly
fall back to `sentry.CurrentHub()` instead of using the intended context from the
original handler. This defeats the purpose of the context propagation fix introduced by
this PR.

Did we get this right? 👍 / 👎 to inform future reviews.

Reference_id: 2648222

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Cloned Handlers Lose Context

The WithAttrs method creates a new eventHandler but doesn't copy the ctx field from the original handler. This breaks context propagation when the handler is cloned, causing h.ctx to be nil in subsequent Handle calls and defeating the purpose of this fix.

slog/sentryslog.go#L198-L209

}
func (h *eventHandler) WithAttrs(attrs []slog.Attr) *eventHandler {
// Create a copy of the groups slice to avoid sharing state
groupsCopy := make([]string, len(h.groups))
copy(groupsCopy, h.groups)
return &eventHandler{
option: h.option,
attrs: appendAttrsToGroup(h.groups, h.attrs, attrs...),
groups: groupsCopy,
}

Fix in Cursor Fix in Web


Bug: Cloned Handlers Lose Context

The WithGroup method creates a new eventHandler but doesn't copy the ctx field from the original handler. This breaks context propagation when the handler is cloned, causing h.ctx to be nil in subsequent Handle calls and defeating the purpose of this fix.

slog/sentryslog.go#L210-L223

}
func (h *eventHandler) WithGroup(name string) *eventHandler {
if name == "" {
return h
}
// Create a copy of the groups slice to avoid modifying the original
newGroups := make([]string, len(h.groups), len(h.groups)+1)
copy(newGroups, h.groups)
newGroups = append(newGroups, name)
return &eventHandler{
option: h.option,

Fix in Cursor Fix in Web


@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.94%. Comparing base (2d82e58) to head (cfc110e).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
slog/sentryslog.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
- Coverage   86.97%   86.94%   -0.03%     
==========================================
  Files          60       61       +1     
  Lines        5557     5563       +6     
==========================================
+ Hits         4833     4837       +4     
- Misses        539      541       +2     
  Partials      185      185              

☔ 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.

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 4dc3b98 into master Nov 13, 2025
18 checks passed
@giortzisg giortzisg deleted the fix/slog-eventhandler branch November 13, 2025 12:44
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.

Error log events are not attached to the correct trace

2 participants