Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
63cc4df to
d644ba0
Compare
d644ba0 to
bf8948b
Compare
bf8948b to
ceeb3d8
Compare
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Mostly lgtm, but I noticed some small things likely worth addressing
| func (h *logHook) SetHubProvider(provider func() *sentry.Hub) { | ||
| h.hubProvider = provider | ||
| h.useCustomProvider = true | ||
| if provider == nil { | ||
| h.SetContextProvider(nil) | ||
| return | ||
| } | ||
| h.SetContextProvider(func() context.Context { | ||
| hub := provider() | ||
| if hub == nil { | ||
| return nil | ||
| } | ||
| return sentry.SetHubOnContext(context.Background(), hub) | ||
| }) | ||
| } |
There was a problem hiding this comment.
l: Why is this still needed if we are getting rid of hubs?
Is it just to satisfy the Hook interface, and if yes, why not just break that interface in this PR?
| func (h *logHook) FlushWithContext(ctx context.Context) bool { | ||
| return h.hubProvider().Client().FlushWithContext(ctx) | ||
| return sentry.ClientFromContext(h.providerContext()).FlushWithContext(ctx) | ||
| } |
There was a problem hiding this comment.
[question] in what case would someone want to use this API?
It seems confusing to have two different contexts. What is the difference between the two contexts and how they are used?
| func TestLogHookContextProviderHonorsExplicitNoopClient(t *testing.T) { | ||
| defaultClient, defaultTransport := setupClientTest() | ||
| ctx, _ := sentry.WithIsolationScope(context.Background()) | ||
| ctx = sentry.ContextWithClient(ctx, sentry.NewNoopClient()) | ||
|
|
||
| hook := NewLogHookFromClient([]logrus.Level{logrus.InfoLevel}, defaultClient) | ||
| hook.SetContextProvider(func() context.Context { return ctx }) | ||
|
|
||
| err := hook.Fire(&logrus.Entry{Level: logrus.InfoLevel, Message: "suppressed"}) | ||
| assert.NoError(t, err) | ||
| assert.True(t, defaultClient.Flush(testutils.FlushTimeout())) | ||
| assert.Empty(t, defaultTransport.Events()) | ||
| } |
There was a problem hiding this comment.
m: I am skeptical we need this test.
At least as far as I can tell, there is no notion of a "global default" Sentry client in Go, at least not after these changes.
So, it is therefore a bit strange to test that a Logrus hook set up with a no-op client would then record events on a totally unrelated test client; even if this client is the default client for tests, it is not a true global default.
It is possible I am missing something, and in that case, I would appreciate if you can let me know where my reasoning is wrong. If I am right, my recommendation would be either to completely remove this test, or to instead narrow it a bit just to check that the logrus hook can be initialized with a no-op client successfully, without then asserting another unrelated client's behavior.
| func contextWithClient(client *sentry.Client) context.Context { | ||
| ctx := sentry.ContextWithScope(context.Background(), sentry.NewScope()) | ||
| return sentry.ContextWithClient(ctx, client) | ||
| } |
There was a problem hiding this comment.
l: I would rename this function to indicate more clearly the difference versus sentry.ContextWithClient (i.e. that a new scope is also created).
| func contextWithClient(client *sentry.Client) context.Context { | |
| ctx := sentry.ContextWithScope(context.Background(), sentry.NewScope()) | |
| return sentry.ContextWithClient(ctx, client) | |
| } | |
| func contextWithClientAndNewScope(client *sentry.Client) context.Context { | |
| ctx := sentry.ContextWithScope(context.Background(), sentry.NewScope()) | |
| return sentry.ContextWithClient(ctx, client) | |
| } |
Although as far as I can tell, the function is only used in one place, so you could also consider removing it.
| // NewWithHub creates a writer using an existing sentry Hub and options. | ||
| func NewWithHub(hub *sentry.Hub, opts Options) (*Writer, error) { | ||
| if hub == nil { | ||
| return nil, errors.New("hub cannot be nil") | ||
| } | ||
| return NewWithContext(sentry.SetHubOnContext(context.Background(), hub), opts) | ||
| } | ||
|
|
There was a problem hiding this comment.
m: This appears to be unused; I am somewhat surprised that there is not a linter to catch this (or perhaps I am mistaken)
| // NewWithHub creates a writer using an existing sentry Hub and options. | |
| func NewWithHub(hub *sentry.Hub, opts Options) (*Writer, error) { | |
| if hub == nil { | |
| return nil, errors.New("hub cannot be nil") | |
| } | |
| return NewWithContext(sentry.SetHubOnContext(context.Background(), hub), opts) | |
| } |
| _, err = NewWithContext(nil, Options{}) // nolint: staticcheck // nil validation | ||
| require.NotNil(t, err) |
There was a problem hiding this comment.
l: I am guessing the linter would catch such a case (that's why we suppress it), and if so, why do we need to test it here?
ceeb3d8 to
7b8a3a5
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 7b8a3a5. Configure here.
7b8a3a5 to
939f803
Compare
939f803 to
0f80bc5
Compare
Route Logrus, Slog, Zap, and Zerolog through context-backed clients and scopes while retaining Hub compatibility until final removal. Rely on the core logger fallback instead of adapter-specific background and provider state. Resolve Logrus and Zap flush clients with the same context fallback as emission, including explicit disabled clients. Clarify flush cancellation versus capture routing and remove the redundant Zerolog context helper.
0f80bc5 to
d7d1275
Compare
| } | ||
| if h.contextProvider != nil { | ||
| if ctx := h.contextProvider(); ctx != nil { | ||
| return ctx |
There was a problem hiding this comment.
Bug: When a contextProvider is set, resolveContext discards the existing entry.Context, causing the loss of per-request data like trace spans added via log.WithContext(requestCtx).
Severity: MEDIUM
Suggested Fix
Modify resolveContext to use the entry.Context as the base and merge it with the context from the contextProvider. This ensures that per-request information, like trace spans, is preserved instead of being completely replaced by the provider's context.
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: logrus/logrusentry.go#L219-L222
Potential issue: When a `contextProvider` is set, the `resolveContext` function returns
the context from the provider directly, completely discarding the existing
`entry.Context`. This is a regression from the previous behavior which used
`entry.Context` as the base. As a result, users who pass request-specific contexts using
`log.WithContext(requestCtx)` will find that important data, such as tracing spans, is
silently dropped from the logs, hindering observability and debugging.
2 participants
Description
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:)Stack created with GitHub Stacks CLI • Give Feedback 💬