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

Allow empty DSN instead of returning an error #22

Closed
wants to merge 1 commit into from

Conversation

chuyeow
Copy link
Contributor

@chuyeow chuyeow commented Jul 3, 2019

Based on

Logger.Println("Sentry client initialized with an empty DSN")
, it looks like the intent is to allow an empty DSN (e.g. when the SENTRY_DSN env var is not set).

The current behavior when the DSN is empty is to return an error from NewClient(). This PR allows the DSN to be empy.

@kamilogorek
Copy link
Contributor

Thanks for filling a PR!

To make it fully functional, we need to also create NoopTransport which implements Transport interface and just logs information about the error being dropped. With the current code, transport will still be installed and it'll try to create Dsn which will fail -

sentry-go/transport.go

Lines 100 to 104 in e3426d1

dsn, err := NewDsn(options.Dsn)
if err != nil {
Logger.Printf("%v\n", err)
return
}

We need to use NoopTransport as the default transport for empty Dsn scenario instead.

@chuyeow
Copy link
Contributor Author

chuyeow commented Jul 8, 2019

With the current code, transport will still be installed and it'll try to create Dsn which will fail

Whoops, I hadn't expected that. I'd assumed that because the global Hub has no Client bound (

var currentHub = NewHub(nil, NewScope()) // nolint: gochecknoglobals
), it will "do the right thing" and do nothing. Can you confirm that my thinking is wrong? I just looked at

sentry-go/sentry.go

Lines 39 to 40 in e3426d1

hub := CurrentHub()
return hub.CaptureException(exception)
and

sentry-go/hub.go

Lines 172 to 174 in e3426d1

client, scope := hub.Client(), hub.Scope()
if client == nil || scope == nil {
return nil
and it looks to me that since hub.Client() is nil, nothing is done.

@kamilogorek
Copy link
Contributor

It binds a client during the first init call, and your change makes empty DSN not return an error, thus it will be bound correctly.

https://github.com/getsentry/sentry-go/blob/master/sentry.go#L12-L20

Current output when calling init will be:

[Sentry] 2019/07/09 15:20:01 Sentry client initialized with an empty DSN
[Sentry] 2019/07/09 15:20:01 [Sentry] DsnParseError: invalid scheme

We need to disable transport (or use noop) and log information about it.

@kamilogorek
Copy link
Contributor

Closed in favor of #27
@chuyeow your commit is preserved :)

@chuyeow
Copy link
Contributor Author

chuyeow commented Aug 5, 2019

@kamilogorek woah thanks for doing that! I couldn’t find the time to work on this before, sorry for the late response.

@kamilogorek
Copy link
Contributor

No worries! Thanks :)

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