develop icon indicating copy to clipboard operation
develop copied to clipboard

startSession and endSession available thru the Static class

Open marandaneto opened this issue 4 years ago • 10 comments

Java does have Sentry.startSession and Sentry.endSession so if one decides to do its own instrumentation, that's possible.

https://develop.sentry.dev/sdk/sessions/#exposed-api

here the exposed API only states available thru the Hub.

Java hides getCurrentHub, it's package-private.

Cocoa has SentrySDK.currentHub() public so one could SentrySDK.currentHub().startSession() but it doesn't have SentrySDK.startSession directly on the Static API.

Is there a reason why? should we unify this?

@bruno-garcia @philipphofmann tagging @mitsuhiko as the initial API design.

marandaneto avatar Jan 20 '21 11:01 marandaneto

On Cocoa, you have both SentrySDK.currentHub, which we use internally quite a lot, and SentrySdk.setCurrentHub(). I don't think that you have many use cases on mobile where you want to change the hub. I don't really know why these two methods are public. Maybe @HazAT or @bruno-garcia you know why these methods are public?

I think it would make sense to offer both Hub.startSession() and SentrySDK.startSession(), but I'm not sure if the above methods should be public.

philipphofmann avatar Jan 21 '21 10:01 philipphofmann

I wish that wasn't public. We have as part of the documented API a way to bind a client to the scope. but swapping out Hubs from the static API isn't something users should be aware as I understand there are no use cases and just makes things harder for us to reason about the code base.

I think it would make sense to offer both Hub.startSession() and SentrySDK.startSession(), but I'm not sure if the above methods should be public.

Makes sense to me. The static SentrySDK.startSession() is just a pass through to the Hub instance set to the static field, so we need that to call into hub.startSession().

bruno-garcia avatar Jan 21 '21 16:01 bruno-garcia

I think the main reason why this is public in Cocoa is that RN, for example, uses it. Still, we could replace the usages in RN with calls to a new method in SentrySDK directly.

I propose the following:

  1. Mark SentrySDK.currentHub and SentrySDK.setCurrentHub as deprecated and remove them in 7.0.0.
  2. Add captureEnvelope and storeEnvelope to SentrySDK.m but not in the header. The hybrid SDKs can create a category to make these two methods accessible. We use this pattern for the test initializers already.
  3. Add both SentrySDK.startSession() and SentrySDK.endSession() to the Static API.

philipphofmann avatar Jan 22 '21 07:01 philipphofmann

Sound good to me. @HazAT you might have thoughts or we're good to go?

bruno-garcia avatar Jan 22 '21 21:01 bruno-garcia

@mitsuhiko any reasons not to add startSession on the static Sentry entrypoint? If not, we'll add it to develop.sentry.dev

bruno-garcia avatar Mar 24 '21 14:03 bruno-garcia

I don’t have strong opinions. It didn’t sound like a super common use case which is why it was on the hub to start.

mitsuhiko avatar Mar 24 '21 18:03 mitsuhiko

This is quite similar in spirit to the set_fingerprint we discussed yesterday, the big difference being Java not exposing a way to get the current hub, thus leaving no way to have methods on the Hub that are not also exposed in the global/static API.

I wonder if instead of documenting startSession as a global API as proposed in https://github.com/getsentry/develop/issues/248#issuecomment-805887468, we could consider making Java more similar to the rest of the SDKs and make getCurrentHub public? What were the downsides that led to getCurrentHub being private?

rhcarvalho avatar May 07 '21 07:05 rhcarvalho

@rhcarvalho pretty much because getCurrentHub isn't in the unified API -> https://develop.sentry.dev/sdk/unified-api/#static-api only the Hub exposes it, but nobody should deal with the Hub directly.

marandaneto avatar May 07 '21 08:05 marandaneto

I agree with @marandaneto that we shouldn't expose getCurrentHub. It was public in Cocoa until 7.0.0. For me, it would be weird to do almost all interactions through the static API, and then for more advanced things, use getCurrentHub. I think all interactions should be possible through the static class to keep it consistent.

philipphofmann avatar May 07 '21 09:05 philipphofmann

@lbloder FYI

markushi avatar Oct 19 '22 14:10 markushi

This was fixed with https://github.com/getsentry/sentry-cocoa/pull/1021 in 2021. We can close this.

philipphofmann avatar May 27 '24 09:05 philipphofmann