Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

by default, don't panic unless there is no alternative #28

Open
bhs opened this issue May 27, 2016 · 5 comments
Open

by default, don't panic unless there is no alternative #28

bhs opened this issue May 27, 2016 · 5 comments

Comments

@bhs
Copy link
Contributor

bhs commented May 27, 2016

Monitoring should always be "first, do no harm".

Though it's formally undefined behavior in OpenTracing, I propose that the basictracer (and thus basictracer-go) default should be to never-panic unless there is truly no alternative; even if that means dropping data.

(Loud logging messages are fine, though)

cc: @bg451 @yurishkuro @jmacd @tschottdorf

@tbg
Copy link
Collaborator

tbg commented May 27, 2016

Fine by me, though I like the option to turn assertions on to catch "undefined" behaviour.

@bhs
Copy link
Contributor Author

bhs commented May 27, 2016

@tschottdorf yes definitely... "default" is the key word here :)

@bhs
Copy link
Contributor Author

bhs commented May 27, 2016

btw, I was imagining something like this:

  • add an (optional) Logger interface to the basictracer.Options struct
    • default to the Go global Logger if it’s not provided
  • only panic when DebugAssertUseAfterFinish is true
  • drop data if need be when we would have otherwise panicked, etc (if there’s no tracer, what better can we do?)
  • and always (regardless of the debug bit) log something scary to the logger where we presently panic… bonus to log stuff about the span in question if it won’t crash the process

I could also imagine s/DebugAssertUseAfterFinish/DebugUsageErrors/, but that's a nit.

@jmacd
Copy link
Collaborator

jmacd commented May 27, 2016

I can't think of many situations where there is truly no alternative to an intentional crash.

@tbg
Copy link
Collaborator

tbg commented May 27, 2016

When there's a data race, there aren't many other sane options as sweeping under the rug is only going to make the bugs more subtle. But with the span pool disabled, (internal) data races aren't an issue any more. However, with the span pool on, I think it should panic when it seens an obviously reused span or the panic will eventually occur in a completely random place.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants