-
Notifications
You must be signed in to change notification settings - Fork 420
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
contrib/net/http: cause the default config to set the analytics rate in spanOpts #564
Conversation
…in spanOpts WrapHandler fails to set the EventSampleRate tag as ServeMux.ServeHTTP does. Rather than relying on ServeMux.ServeHTTP and WrapHandler to individually set the tag, we will have the defaults function set the tag in the config if the global analytics rate is not NaN.
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.
This is good. Can we please add a regression test and fix CI?
Fixed the tests, added testing for regression that I have confirmed fails without this patch. |
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.
You’re right. This gives better coverage, but can we tidy up a bit more?
contrib/net/http/http_test.go
Outdated
assert.Len(t, spans, 1) | ||
s := spans[0] | ||
assert.Equal(t, rate, s.Tag(ext.EventSampleRate)) | ||
asserts := []func(t *testing.T, mt mocktracer.Tracer, rate interface{}, opts ...Option){ |
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.
Would a map with a string key help be more descriptive?
contrib/net/http/http_test.go
Outdated
t.Run("defaults", func(t *testing.T) { | ||
mt := mocktracer.Start() | ||
defer mt.Stop() | ||
for assert := range asserts { |
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 think we are overusing and shadowing this name “assert”. It’s a test, maybe “test” is better? You could use the suggested map’s key string as the name
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.
You're right. There are too many things called "assert." I think a map with a string name is better.
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.
Really nice work Kyle.
The tests are failing due to a grpc library issue. I'll leave this here for now and address that issue in a separate PR. |
The issue was fixed upstream. See: #566 |
…in spanOpts (DataDog#564) WrapHandler fails to set the EventSampleRate tag as ServeMux.ServeHTTP does. Rather than relying on ServeMux.ServeHTTP and WrapHandler to individually set the tag, we will have the defaults function set the tag in the config if the global analytics rate is not NaN.
WrapHandler fails to set the EventSampleRate tag as ServeMux.ServeHTTP does.
Rather than relying on ServeMux.ServeHTTP and WrapHandler to individually set
the tag, we will have the defaults function set the tag in the config if the global analytics
rate is not NaN.