Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

with_scope sometimes provides a scope = nil if there is no client. Other SDKs don't do this. #2010

Closed
untitaker opened this issue Feb 22, 2023 · 10 comments
Assignees

Comments

@untitaker
Copy link
Member

untitaker commented Feb 22, 2023

Issue Description

Sentry.with_scope calls the passed block with nil if the SDK is not initialized. This is inconsistent with the Python SDK.

Reproduction Steps

Sentry.with_scope do |scope|
  scope.set_tag("reblog_of_id", 123)
  Sentry.capture_message("dropped a viral post")
end

run this code with the SDK imported but not initialized.

This code crashes if the Sentry SDK is set up, because scope is nil.

Expected Behavior

I expect scope to be non-nullable, i.e. there is a current scope at all times regardless of the SDK's state. I expect this because I am used to this behavior from the Python SDK, but I also just like this behavior more because there's less of a chance my added instrumentation code crashes depending on the environment I deploy it in.

Actual Behavior

The code crashes.

Further comment: It seems to me that the Ruby SDK internally diverges significantly from Python in how it manages current hubs, current scope etc. The Python SDK always has a current hub and a current scope no matter how the SDK is configured, no matter which thread the current hub is fetched from. Ruby OTOH seems to create its first hub upon initialization.

I imagine that causes further issues, for example in the Ruby SDK, can I even set tags before I initialize the SDK? In Python I can.

Ruby Version

3.2.1, but it is irrelevant

SDK Version

5.7.0

Integration and Its Version

rails and sidekiq, all on 5.7.0

Sentry Config

that's the point, there's none

@st0012
Copy link
Collaborator

st0012 commented Feb 25, 2023

I agree that it's better to have scope always present, but:

  • We always advise Ruby users to always initialise their SDKs early on and control event sending with config.enabled_environments.
  • Also, in most Ruby libraries it's expected that just loading the files won't create the supported object structure that comes with it. That is, the above practice makes sense to Ruby devleopers.

So from my experience, users aren't bothered by this behaviour much.

And regarding the change itself, I think it "may" be possible, but we need to clarify a few things first:

  • I assume we'll initialise the Hub structure with a default/null Configuration object? And when the Sentry.init does get called, do we throw away the previous one and do that again with user configs?
    • It sounds like in most apps, we'll create Hub (and its dependencies) twice?
  • If the user doesn't call Sentry.init, but has SENTRY_DSN env, is the SDK allowed to send events?

Finally, given this will be a large change and almost certainly would introduce problems when it's rolled out, I think we should not proceed until we see other clear benefits.

@github-actions
Copy link

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@untitaker
Copy link
Member Author

untitaker commented Mar 19, 2023

I don't see the strong reason for diverging from common behavior. We don't recommend always initializing any other sdk all the time to avoid nullpointers in with-scope.

The wizards and quickstart documentation out there is largely phrased the same across platforms, and I followed the rails one to the letter, yet still ended up in this state where tests break.

The python sdk is a decent reference for exact behavior that I believe is also present in js. Yes there's always a hub with a scope present, so setting tags works before and after client initialization.

@sl0thentr0py
Copy link
Member

@st0012 I guess we can line this up for the major? (unless you're really opposed to it)

@st0012
Copy link
Collaborator

st0012 commented Mar 20, 2023

If we only look at the Ruby SDK itself, I don't see much benefit for such change. But if aligning SDKs' behaviours has high priority, sure let's do it 👍

And given you're more familiar with the Python SDK, can you help me check these behaviours? 🙏

  • I assume we'll initialise the Hub structure with a default/null Configuration object? And when the Sentry.init does get called, do we throw away the previous one and do that again with user configs?
    • It sounds like in most apps, we'll create Hub (and its dependencies) twice?
  • If the user doesn't call Sentry.init, but has SENTRY_DSN env, is the SDK allowed to send events?
  • Do we keep tags assigned before the SDK is initialised? Or we just don't want to cause errors in that case?

@sl0thentr0py
Copy link
Member

I think the main benefit is as @untitaker said to not break any user code even if Sentry.init is not called and they use some other Sentry.function calls.

The following is basically what is missing since this runs irrespective of whether init is called or not on module import.
https://github.com/getsentry/sentry-python/blob/5e7627ca438f3991afad9576f4e44753f70a9803/sentry_sdk/hub.py#L751-L752

@untitaker
Copy link
Member Author

I assume we'll initialise the Hub structure with a default/null Configuration object? And when the Sentry.init does get called, do we throw away the previous one and do that again with user configs?

Every thread has a Hub at all times, i.e. "get current hub" lazily creates a hub on the thread local based on the "main hub" (= hub on the main thread) if there is none

Hub.current is non-nullable in Python

Then, the hub has a stack of (nullable_client, scope) tuples. with_scope/push_scope does not deepcopy the client, only the scope

the stack cannot be empty, it is initialized with [(None, Scope())]

sentry_sdk.init() replaces the client on the top-layer of the stack with a new one. the scope stays the same

this results in the external behavior changes:

  1. I can run set_tag before I initialize. So I can set_tag("rails_env", "production") in one initializer file and configure teh SDK in another, and I don't have to worry about initialization order.
  2. calling init() inside with_scope temporarily initializes the SDK, and after the with_scope block, the sdk is uninitialized again

@untitaker
Copy link
Member Author

untitaker commented Mar 20, 2023

I assume we'll initialise the Hub structure with a default/null Configuration object? And when the Sentry.init does get called, do we throw away the previous one and do that again with user configs?

Configuration object doesn't exist in Python, I suppose that's equivalent to Client

It sounds like in most apps, we'll create Hub (and its dependencies) twice?

init() mutates stack of the current hub, it does not create/destroy hubs. it does create a new Client.

If the user doesn't call Sentry.init, but has SENTRY_DSN env, is the SDK allowed to send events?

no, I have to call sentry_sdk.init() without args to initialize the SDK. if i do not pass a dsn arg, the envvar is honored. if I pass an empty string, I believe the sdk is effectively disabled

Do we keep tags assigned before the SDK is initialised? Or we just don't want to cause errors in that case?

yes


by the way, i have no strong opinion on whether all SDKs should be aligned to this level of detail -- just describing what python does

I do sort of care about being able to set_tag before the sdk is initialized, and I strongly feel that it should not be possible to crash user code by having forgotten to initialize the SDK (since this can happen in a completely different part of the codebase)

that said I am no longer involved with SDK development and cannot make any calls here. Just commenting as a user and former maintainer of py sdk

@sl0thentr0py
Copy link
Member

fwiw these are not really relevant to ruby

I can run set_tag before I initialize. So I can set_tag("rails_env", "production") in one initializer file and configure teh SDK in another, and I don't have to worry about initialization order.

the order problem is solved separately in ruby/rails land and this is a non-issue

calling init() inside with_scope temporarily initializes the SDK, and after the with_scope block, the sdk is uninitialized again

again opaque behaviour that no one really needs tbh

it's fine to simplify this discussion to "don't break user code when not initialized", we don't really need to talk about random edge case behaviours.

@sl0thentr0py
Copy link
Member

we will be re-working the scope/hub system completely anyway so this is kinda stale now, closing.

@sl0thentr0py sl0thentr0py closed this as not planned Won't fix, can't repro, duplicate, stale Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants