Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

startSession and endSession available thru the Static class #248

Closed
marandaneto opened this issue Jan 20, 2021 · 11 comments
Closed

startSession and endSession available thru the Static class #248

marandaneto opened this issue Jan 20, 2021 · 11 comments

Comments

@marandaneto
Copy link
Contributor

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 marandaneto added documentation Improvements or additions to documentation release health labels Jan 20, 2021
@philipphofmann
Copy link
Member

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.

@bruno-garcia
Copy link
Member

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().

@philipphofmann
Copy link
Member

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.

@bruno-garcia
Copy link
Member

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

@bruno-garcia
Copy link
Member

bruno-garcia commented Mar 24, 2021

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

@mitsuhiko
Copy link
Contributor

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.

@rhcarvalho
Copy link
Contributor

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 #248 (comment), 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?

@marandaneto
Copy link
Contributor Author

@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.

@philipphofmann
Copy link
Member

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.

@markushi
Copy link
Member

@lbloder FYI

@philipphofmann
Copy link
Member

This was fixed with getsentry/sentry-cocoa#1021 in 2021. We can close this.

@github-project-automation github-project-automation bot moved this from Backlog to Done in Mobile SDKs May 27, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
Archived in project
Development

No branches or pull requests

6 participants