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

feat: Add flush #2140

Merged
merged 25 commits into from
Sep 19, 2022
Merged

feat: Add flush #2140

merged 25 commits into from
Sep 19, 2022

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Sep 12, 2022

📜 Description

Add blocking flush to send out all envelope items.

This PR is based on #2169.

💡 Motivation and Context

Fixes GH-1013

💚 How did you test it?

Unit tests and real devices.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

Add blocking flush to send out all envelope items.

Fixes GH-1013
@github-actions
Copy link

github-actions bot commented Sep 12, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against b505dff

@github-actions
Copy link

github-actions bot commented Sep 12, 2022

Performance metrics 🚀

Plain With Sentry Diff
Startup time (ms) 1213.22 1235.58 22.36
Size (bytes) 20997 336832 315835

@philipphofmann philipphofmann marked this pull request as ready for review September 15, 2022 09:24
We need SentryTime also on tvOS so we can make it
available everywhere.
@philipphofmann philipphofmann changed the base branch from master to ref/time-available-for-tvos September 15, 2022 09:59
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I have only one observation regarding comments.

Comment on lines 117 to 119
* Waits synchronously for the SDK to flush out all queued and cached items. Returns if the SDK
* doesn't complete the flush before the specified timeout period has elapsed or if there is no
* internet connection. The SDK doesn't dispose the client or the hub.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: The description looks a bit confusion "Returns if the SDK doesn't complete...", if everything work then this function does not return? I know it does, but the phrasing is odd for me (maybe just me). Maybe we could have the error parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Is it better now?

Base automatically changed from ref/time-available-for-tvos to master September 19, 2022 08:10
@philipphofmann philipphofmann merged commit bb940ad into master Sep 19, 2022
@philipphofmann philipphofmann deleted the feat/flush branch September 19, 2022 12:04
kevinrenskers added a commit that referenced this pull request Sep 19, 2022
* master:
  meta: Automatically manage signing (#2184)
  fix: Support for arm64 architecture (#2185)
  meta: Contributing md lint fixes (#2186)
  feat: Add flush (#2140)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SentrySDK.flush()
3 participants