Repository navigation
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
7b8a3a5 to
939f803
Compare
939f803 to
0f80bc5
Compare
0f80bc5 to
d7d1275
Compare
82b9c2c to
ea8887b
Compare
ea8887b to
b71dd8b
Compare
e11b3f0 to
17152f8
Compare
17152f8 to
c815126
Compare
c815126 to
4636c6d
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.
4636c6d to
d7d2cbd
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 d7d2cbd. Configure here.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Stamping, but some previous comments appear to still be open. Would appreciate if you can take a look before the merge 🙏
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 💬