-
Notifications
You must be signed in to change notification settings - Fork 421
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
use a seeded/thread-safe random number generator for span ids #48
Conversation
This way we get the right error messages when the assertion fails.
Fixes #46. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good globally, left a remark about side effects of an unexpected panic() in tracing code.
@@ -52,6 +55,12 @@ func NewTracer() *Tracer { | |||
|
|||
// NewTracerTransport create a new Tracer with the given transport. | |||
func NewTracerTransport(transport Transport) *Tracer { | |||
randSource, err := newRandSource() | |||
if err != nil { | |||
panic(fmt.Sprintf("cannot create random source: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried with panicking here, if, for some reason, creating the rand source we should probably log it, make it blatant there's a problem, and make all tracing no-op so that no span/trace are sent, making it clear something is going wrong. If we do panic here for real, we might stop user code and create an outage on our behalf. Generally speaking, panicking on totally unexpected thing is OK, but here we're hooking ourselves on existing code, trying to be as furtive as possible.
@@ -60,7 +60,7 @@ func TestTracesAgentIntegration(t *testing.T) { | |||
for _, tc := range testCases { | |||
transport := newHTTPTransport(defaultHostname, defaultPort) | |||
response, err := transport.SendTraces(tc.payload) | |||
assert.Nil(err) | |||
assert.NoError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.