Conversation
4d788fd to
deeab0c
Compare
deeab0c to
7e6baff
Compare
7e6baff to
51c535e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 51c535e. Configure here.
51c535e to
d7b1f55
Compare
d7b1f55 to
45876fc
Compare
45876fc to
71754bd
Compare
71754bd to
4ec37b3
Compare
4ec37b3 to
c4e83dd
Compare
c4e83dd to
c2a58ea
Compare
There was a problem hiding this comment.
The changes mostly look okay, but it seems this PR contains more changes than are suggested by the PR description. Was there perhaps a problem when rebasing the stack? Or these are all intended to be here?
Edit: Nvm, I re-read the description and suppose everything is in scope 😅
There was a problem hiding this comment.
What was this file's purpose? Was it for enable_logs == false?
| type panicStringer struct{} | ||
|
|
||
| func (panicStringer) String() string { | ||
| panic("disabled logs must not format values") | ||
| } | ||
|
|
||
| // flushFromContext flushes the client from the given context. | ||
| func flushFromContext(ctx context.Context, timeout time.Duration) { | ||
| hub := GetHubFromContext(ctx) | ||
| if hub == nil { | ||
| hub = CurrentHub() | ||
| } | ||
| hub.Flush(timeout) | ||
| ClientFromContext(ctx).Flush(timeout) | ||
| } | ||
|
|
||
| func TestDisabledLoggerDoesNotFormatValues(_ *testing.T) { | ||
| ctx := ContextWithClient(context.Background(), NewNoopClient()) | ||
| logger := NewLogger(ctx) | ||
|
|
||
| logger.Info().Emit(panicStringer{}) | ||
| logger.Info().Emitf("%v", panicStringer{}) |
| fallbackCtx := m.fallbackCtx | ||
| scope := ScopeFromContext(ctx) | ||
| if scope == nil { | ||
| scope = ScopeFromContext(fallbackCtx) | ||
| } else { | ||
| fallbackCtx = nil | ||
| } | ||
| if options.scope != nil { | ||
| scope = options.scope | ||
| } |
There was a problem hiding this comment.
l: It looks like we had similar logic in log.go. Perhaps we can extract it into a function?
| setString("sentry.release", client.options.Release) | ||
| setString("sentry.environment", client.options.Environment) | ||
| setString("sentry.server.address", serverAddress) | ||
| setString("sentry.sdk.name", sdkName) | ||
| setString("sentry.sdk.version", client.sdkVersion) | ||
| setString("user.id", scope.user.ID) | ||
| setString("user.name", scope.user.Name) | ||
| setString("user.email", scope.user.Email) |
There was a problem hiding this comment.
m: It would probably be a good idea to extract these attribute names into constants rather than hardcoding them
Resolve log, metric, and propagation data from context-backed clients and scopes, and expose context-based trace header helpers while retaining the existing Hub compatibility APIs.
c2a58ea to
aeb2126
Compare
| scope = ScopeFromContext(fallbackCtx) | ||
| } else { | ||
| fallbackCtx = nil | ||
| } | ||
| if options.scope != nil { |
There was a problem hiding this comment.
Bug: When a scope is found in the primary context, the fallback context is set to nil, preventing a search for trace information and potentially losing existing trace context.
Severity: MEDIUM
Suggested Fix
Do not set fallbackCtx to nil when a scope is found in the primary context. Allow activeTraceFromContexts to search both the primary and fallback contexts to ensure trace information is not lost, preserving the behavior of the previous implementation.
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: metrics.go#L79-L83
Potential issue: When a scope is found in the primary context `ctx`, the `fallbackCtx`
is explicitly set to `nil`. This prevents the subsequent call to
`activeTraceFromContexts` from searching the fallback context for an active span or
trace. This can cause a loss of trace context in scenarios where a logger or meter is
initialized with a context containing a span, and later used via `.WithCtx()` with a
different context that has a scope but no span. The trace from the original context will
be dropped, silently breaking trace continuity.
Also affects:
log.go:90~94
Description
This adds new trace propagation APIs that resolve with context. Also changes logs and metrics to resolve scope through context rather than the old hub API.
Issues
Changelog Entry Instructions
To add a custom changelog entry, uncomment the section above. Supports:
For more details: custom changelog entries
Reminders
feat:,fix:,ref:,meta:)