-
Notifications
You must be signed in to change notification settings - Fork 444
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/bradfitz/gomemcache: add memcache tracer #291
Conversation
93afaa5
to
39fa1ff
Compare
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.
An initial round of comments.
// you can use WithContext to set the parent span | ||
mc.WithContext(ctx).Set(&memcache.Item{Key: "my key", Value: []byte("my value")}) | ||
|
||
span.Finish() |
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.
We usually defer
this in most examples.
) | ||
|
||
func Example() { | ||
span, ctx := tracer.StartSpanFromContext(context.Background(), "example", |
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.
Can we make up some better imaginary names than "example" for span and service? Let's think of a basic simplistic use-case or "hello world" example.
// the existing memcache client doesn't support context, but may in the | ||
// future, so we do a runtime check to detect this | ||
mc := c.Client | ||
if hasWithContext, ok := (interface{})(c.Client).(interface { |
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 (interface{})
hack is nasty. But it's ok, it's a good approach.
// the existing memcache client doesn't support context, but may in the | ||
// future, so we do a runtime check to detect this | ||
mc := c.Client | ||
if hasWithContext, ok := (interface{})(c.Client).(interface { |
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.
hasWithContext
sounds like a boolean. Can we use something else (some examples: v
, wc
)
// startSpan starts a span from the context set with WithContext. | ||
func (c *Client) startSpan(resourceName string) ddtrace.Span { | ||
span, _ := tracer.StartSpanFromContext(c.context, operationName, | ||
tracer.ServiceName(c.cfg.serviceName), |
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.
tracer.SpanType(ext.SpanTypeMemcached)
} | ||
defer li.Close() | ||
|
||
go func() { |
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.
Can we move this to a separate function and have a unit test for it?
ff0e63a
to
dba173a
Compare
spans := mt.FinishedSpans() | ||
assert.Len(t, spans, 1) | ||
validateMemcacheSpan(t, spans[0], "Add") | ||
}) |
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.
please add an empty line between
}) | ||
} | ||
|
||
func makeFakeServer(t *testing.T) net.Listener { |
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.
We should write a test for this helper. Our main tests rely on it.
…test for fake server
dba173a
to
3a1b9d4
Compare
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.
Thanks!
@@ -30,6 +30,7 @@ jobs: | |||
DD_APM_ENABLED: "true" | |||
DD_BIND_HOST: "0.0.0.0" | |||
DD_API_KEY: invalid_key_but_this_is_fine | |||
- image: memcached:1.5.9 |
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 might have already noticed, but in case not, we're using resource_class: xlarge
. I'm not sure how long that'll last us if we keep adding new services. It's 8 CPUs and 16GB.
} | ||
|
||
func TestMemcacheIntegration(t *testing.T) { | ||
if _, ok := os.LookupEnv("INTEGRATION"); !ok { |
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.
👍
Fixes #290
This adds a memcache trace library. We wrap the
memcache.Client
and return a new struct with all the same methods:We also add one additional method,
WithContext
:This is similar to how the redis library implements
Context
.