Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
rfc(feature): SDK Lifecycle Hooks #34
rfc(feature): SDK Lifecycle Hooks #34
Changes from all commits
67ad7ea
4ae43a7
a6b7b99
a2c88e1
18d4e7a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Mobile, take a screenshot or attach the view hierarchy when capturing an exception. Could we replace our custom logic of attachment processors with that, @AbhiPrasad? We call some code that returns attachments and then bundle it into an envelope when capturing exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! We could for sure replace that with this, will add your example to the list!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbhiPrasad, why did we put the hooks on the client and not the options as our existing hooks
beforeSend
,beforeBreadcrumb
, etc.?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not meant to be replacements for the top level hooks we have for
beforeSend
orbeforeBreadcrumb
for the time being, since the hook names are dynamic strings.The hooks are on the client so they can be registered dynamically. This is basically the event emitter pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to put this info in the RFC or the develop docs, as this was unclear to me when reading the RFC.
I didn't write JavaScript for 6 years or so, but event emitter still rings a bell now. For statically typed languages, we might have to adapt them to the language. For Swift and Java, it would maybe be something like
addObserver
andnotifyObserver
, or maybe we just stick to the JavaScript syntax. Thanks for explaining.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused, who implements this
Client
? What exactly the end-user interacts with?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for showing an interest in our RFC process! 🚀
This is meant to be the client implementation we have in each SDK, see https://develop.sentry.dev/sdk/unified-api/#client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to think a layer higher and across multiple languages and clients. Sorry for the confusion, and thank you so much for your response!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cleptric, I also need some clarification. Does this mean I need to implement a client when I want to add lifecycle hooks? Can you share the code a user must write when implementing a lifecycle hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
on
andemit
hooks will be defined on the client interface (much likeflush
andcapture_event
are at the moment).Usually users (if writing custom clients) are inheriting from a base client implementation anyway, so they should get the hooks functionality out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, @AbhiPrasad 😃 .
On mobile, hardly anybody implements a custom client. Most users interact with the static API. Implementing a custom client and binding this to the hub, then binding the hub to the static API, will not be the cleanest API on Mobile. Our users usually only call
SentrySDK.start
with the options, and that's it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's about hooks for
startSession
andendSession
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not an exhaustive list, but I will add those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also add our existing hooks as
beforeSend
,beforeTransaction
,beforeBreadcrumb
, etc. to have one way to define all hooks, and we should keep the existing hooks for backward compatibility. Otherwise, users need to use two different APIs for a similar use case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense to me - can add some notes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a hook for
beforeEnvelope
? Can we make it internal at least? AFAIK this is not exposed for clients and the whole envelope impl. is marked as internal in Mobile SDKs at least.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can make some hooks internal. I will clarify this in the RFC.