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

Unified API Client.Close() #111

Open
rhcarvalho opened this issue Dec 6, 2019 · 4 comments
Open

Unified API Client.Close() #111

rhcarvalho opened this issue Dec 6, 2019 · 4 comments

Comments

@rhcarvalho
Copy link
Contributor

The Shutdown and Draining docs page for Go suggests using sentry.Flush where other SDKs use Close.

Then it says:

After shutdown the client cannot be used any more so make sure to only do that right before you shut down the application.

That is incorrect docs, since it doesn't match what Flush does.

We should implement Close and fix the docs to use it.

As seen in recent issues, Flush is often not the answer and is being misused.

@rhcarvalho rhcarvalho added this to To do in Sentry SDK for Go via automation Dec 6, 2019
@kamilogorek
Copy link
Contributor

kamilogorek commented Dec 6, 2019

Agree, I missed that. Close is basically Flush + client.Enabled = false

@rhcarvalho rhcarvalho added this to the 0.5.0 milestone Dec 28, 2019
@rhcarvalho
Copy link
Contributor Author

Close would also involve releasing resources. Like os.File.Close, http.Response.Body.Close or context.CancelFunc do.

Potential signatures:

  1. Explicit timeout argument

    // Close shuts down the SDK and blocks until all outstanding events have been
    // sent or the timeout is reached. Unlike Flush, Close releases resources
    // associated with the SDK. Code should typically defer a call to Close after
    // Init.
    func Close(timeout time.Duration) {}
    
  2. Timeout from Client.Options

    Some SDKs (e.g. sentry-dotnet) take the timeout from configuration.

    // Close shuts down the SDK and blocks until all outstanding events have been
    // sent or ShutdownTimeout is reached. Unlike Flush, Close releases resources
    // associated with the SDK. Code should typically defer a call to Close after
    // Init.
    func Close() {}
    

    Alternatively, we can implement io.Closer:

    func Close() error {}
    

@rhcarvalho rhcarvalho removed this from the v0.5.0 milestone Jan 29, 2020
@rhcarvalho
Copy link
Contributor Author

While this is part of the Unified SDK spec, this is not a critical feature.

Originally I wanted to bundle this together with improvements to Flush and updating docs.
Now I realize we do not need to add this right now, we might as well take the time to learn what would be the best signature if we end up adding it later.

Considering recent conversations with @bruno-garcia and having in mind his influence to remind me that "when in doubt, leave it out", I decided to leave it out for now.

Correct programs can be written using sentry.Flush.

What sentry.Close would add is resource cleanup, in particular terminating a worker goroutine in the default transport. That termination is mostly interesting for tests and for programs that try to "add" and "remove" the SDK in runtime. The former is unknown of at the time, and would require extra work to make possible (hint: there is more global state in the SDK).

Note that in other languages, once the SDK is initialized, there is no such option to remove all of its traces (e.g., we can't safely reverse monkey patches, though that's not a thing in Go).

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

3 participants