getsentry / sentry-go

The official Go SDK for Sentry (sentry.io)
https://docs.sentry.io/platforms/go/
MIT License
924 stars 213 forks source link

Unified API Client.Close() #111

Open rhcarvalho opened 4 years ago

rhcarvalho commented 4 years ago

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.

kamilogorek commented 4 years ago

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

rhcarvalho commented 4 years ago

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 commented 4 years ago

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