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

docs: Update Flush documentation #153

Merged
merged 1 commit into from
Feb 2, 2020

Conversation

rhcarvalho
Copy link
Contributor

Related to #106.

@rhcarvalho
Copy link
Contributor Author

There is one nuance here that is not addressed:

  • In an ideal program, there needs to be one call to Flush per transport upon termination.
  • All the multiple Flush methods eventually call Transport.Flush. Transport is the actual thing that needs flushing.
  • If the program has many clients/transports, calling sentry.Flush only flushes the top-level client/transport pair in the internal stack of the current hub.

As a user, one might feel overwhelmed with so many Flush methods and understanding when what needs to be called.

@kamilogorek @HazAT @bruno-garcia do you have any opinions or advice on how to deal with that complexity?

BTW should sentry.Flush and Hub.Flush flush all clients on its stack?

@bruno-garcia
Copy link
Member

IMHO User only knows about Sentry.flush and worst case SentryClient.flush if they are working with the client directly (outside the SDK.init/Hub). No one is flushing a transport directly. To my knowledge transport is an implementation. Sure its flush leaks out all the way to the static API but we optimize for simplicity.

I agree it's no ideal, the design. We've discussed this but no one put the effort to propose a solution.

Also, I rather not focus on Flush. For the longest time I didn't want to add Flush at all. Only when serverless came along I ran out of arguments against it.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

lgtm. all things considered

@rhcarvalho
Copy link
Contributor Author

IMHO User only knows about Sentry.flush and worst case SentryClient.flush if they are working with the client directly (outside the SDK.init/Hub)

I wish this was simpler too.

If there is only ever one client, then sentry.Flush does the job.
If new clients are created, then more Flush calls need to be sprinkled around for correctness. We need a clear example for this usage.

@rhcarvalho
Copy link
Contributor Author

Also, I rather not focus on Flush. For the longest time I didn't want to add Flush at all. Only when serverless came along I ran out of arguments against it.

Unlike C# that has Disposable and other languages that have hooks to call Flush under the hood, in Go there are no magical hooks. Programs must call Flush explicitly.

What we're trying to clarify here is that the typical use involves a single call when the program terminates, instead of multiple calls, one after every Capture* call.

With this and our recent face to face conversations, and having in mind your influence to remind me that "when in doubt, leave it out", I decided to postpone and deprioritize adding sentry.Close (#111).

@rhcarvalho rhcarvalho merged commit d645d79 into getsentry:master Feb 2, 2020
@rhcarvalho rhcarvalho deleted the flush-docs branch February 2, 2020 19:43
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.

2 participants