From 3999161937e5d8d5b086cddcdece727df7621435 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Mon, 18 May 2020 11:32:52 -0500 Subject: [PATCH] Revert "contrib/bradfitz/gomemcache: trace item info for memcached operations (#642)" (#663) This reverts commit 3f45f6d1a30653f2616fcb2df7affa978685c887. This can be problematic for multiple reasons such as producing unexpectedly large quantities of data, and possible security issues. We do not want to provide APIs with unexpected or potentially dangerous behavior. --- .../bradfitz/gomemcache/memcache/memcache.go | 35 ------------------- .../gomemcache/memcache/memcache_test.go | 10 ++---- .../bradfitz/gomemcache/memcache/option.go | 9 ----- internal/version/version.go | 2 +- 4 files changed, 4 insertions(+), 52 deletions(-) diff --git a/contrib/bradfitz/gomemcache/memcache/memcache.go b/contrib/bradfitz/gomemcache/memcache/memcache.go index d981183b17..fe6d458ff3 100644 --- a/contrib/bradfitz/gomemcache/memcache/memcache.go +++ b/contrib/bradfitz/gomemcache/memcache/memcache.go @@ -80,11 +80,6 @@ func (c *Client) startSpan(resourceName string) ddtrace.Span { func (c *Client) Add(item *memcache.Item) error { span := c.startSpan("Add") err := c.Client.Add(item) - span.SetTag("item.key", item.Key) - if c.cfg.withValueTags { - span.SetTag("item.value", item.Value) - } - span.SetTag("item.expiration", item.Expiration) span.Finish(tracer.WithError(err)) return err } @@ -93,11 +88,6 @@ func (c *Client) Add(item *memcache.Item) error { func (c *Client) CompareAndSwap(item *memcache.Item) error { span := c.startSpan("CompareAndSwap") err := c.Client.CompareAndSwap(item) - span.SetTag("item.key", item.Key) - if c.cfg.withValueTags { - span.SetTag("item.value", item.Value) - } - span.SetTag("item.expiration", item.Expiration) span.Finish(tracer.WithError(err)) return err } @@ -106,11 +96,6 @@ func (c *Client) CompareAndSwap(item *memcache.Item) error { func (c *Client) Decrement(key string, delta uint64) (newValue uint64, err error) { span := c.startSpan("Decrement") newValue, err = c.Client.Decrement(key, delta) - span.SetTag("item.key", key) - if c.cfg.withValueTags { - span.SetTag("item.value.before", newValue-delta) - span.SetTag("item.value.after", newValue) - } span.Finish(tracer.WithError(err)) return newValue, err } @@ -119,7 +104,6 @@ func (c *Client) Decrement(key string, delta uint64) (newValue uint64, err error func (c *Client) Delete(key string) error { span := c.startSpan("Delete") err := c.Client.Delete(key) - span.SetTag("item.key", key) span.Finish(tracer.WithError(err)) return err } @@ -144,7 +128,6 @@ func (c *Client) FlushAll() error { func (c *Client) Get(key string) (item *memcache.Item, err error) { span := c.startSpan("Get") item, err = c.Client.Get(key) - span.SetTag("item.key", key) span.Finish(tracer.WithError(err)) return item, err } @@ -153,7 +136,6 @@ func (c *Client) Get(key string) (item *memcache.Item, err error) { func (c *Client) GetMulti(keys []string) (map[string]*memcache.Item, error) { span := c.startSpan("GetMulti") items, err := c.Client.GetMulti(keys) - span.SetTag("item.keys", keys) span.Finish(tracer.WithError(err)) return items, err } @@ -162,11 +144,6 @@ func (c *Client) GetMulti(keys []string) (map[string]*memcache.Item, error) { func (c *Client) Increment(key string, delta uint64) (newValue uint64, err error) { span := c.startSpan("Increment") newValue, err = c.Client.Increment(key, delta) - span.SetTag("item.key", key) - if c.cfg.withValueTags { - span.SetTag("item.value.before", newValue-delta) - span.SetTag("item.value.after", newValue) - } span.Finish(tracer.WithError(err)) return newValue, err } @@ -175,11 +152,6 @@ func (c *Client) Increment(key string, delta uint64) (newValue uint64, err error func (c *Client) Replace(item *memcache.Item) error { span := c.startSpan("Replace") err := c.Client.Replace(item) - span.SetTag("item.key", item.Key) - if c.cfg.withValueTags { - span.SetTag("item.value", item.Value) - } - span.SetTag("item.expiration", item.Expiration) span.Finish(tracer.WithError(err)) return err } @@ -188,11 +160,6 @@ func (c *Client) Replace(item *memcache.Item) error { func (c *Client) Set(item *memcache.Item) error { span := c.startSpan("Set") err := c.Client.Set(item) - span.SetTag("item.key", item.Key) - if c.cfg.withValueTags { - span.SetTag("item.value", item.Value) - } - span.SetTag("item.expiration", item.Expiration) span.Finish(tracer.WithError(err)) return err } @@ -201,8 +168,6 @@ func (c *Client) Set(item *memcache.Item) error { func (c *Client) Touch(key string, seconds int32) error { span := c.startSpan("Touch") err := c.Client.Touch(key, seconds) - span.SetTag("item.key", key) - span.SetTag("item.expiration", seconds) span.Finish(tracer.WithError(err)) return err } diff --git a/contrib/bradfitz/gomemcache/memcache/memcache_test.go b/contrib/bradfitz/gomemcache/memcache/memcache_test.go index 2225e2282c..58bb87e891 100644 --- a/contrib/bradfitz/gomemcache/memcache/memcache_test.go +++ b/contrib/bradfitz/gomemcache/memcache/memcache_test.go @@ -38,7 +38,7 @@ func TestMemcacheIntegration(t *testing.T) { } func testMemcache(t *testing.T, addr string) { - client := WrapClient(memcache.New(addr), WithServiceName("test-memcache"), WithValueTags()) + client := WrapClient(memcache.New(addr), WithServiceName("test-memcache")) defer client.DeleteAll() validateMemcacheSpan := func(t *testing.T, span mocktracer.Span, resourceName string) { @@ -76,9 +76,8 @@ func testMemcache(t *testing.T, addr string) { err := client. WithContext(ctx). Add(&memcache.Item{ - Key: "key2", - Value: []byte("value2"), - Expiration: 10, + Key: "key2", + Value: []byte("value2"), }) assert.Nil(t, err) @@ -90,9 +89,6 @@ func testMemcache(t *testing.T, addr string) { assert.Equal(t, span, spans[1]) assert.Equal(t, spans[1].TraceID(), spans[0].TraceID(), "memcache span should be part of the parent trace") - assert.Equal(t, "key2", spans[0].Tag("item.key")) - assert.Equal(t, []byte("value2"), spans[0].Tag("item.value")) - assert.Equal(t, int32(10), spans[0].Tag("item.expiration")) }) } diff --git a/contrib/bradfitz/gomemcache/memcache/option.go b/contrib/bradfitz/gomemcache/memcache/option.go index 00303294c6..73374c94a0 100644 --- a/contrib/bradfitz/gomemcache/memcache/option.go +++ b/contrib/bradfitz/gomemcache/memcache/option.go @@ -17,7 +17,6 @@ const ( type clientConfig struct { serviceName string analyticsRate float64 - withValueTags bool } // ClientOption represents an option that can be passed to Dial. @@ -58,11 +57,3 @@ func WithAnalyticsRate(rate float64) ClientOption { } } } - -// WithValueTags specifies whether values assigned to keys in memcache operations -// should be added to spans as tags. -func WithValueTags() ClientOption { - return func(cfg *clientConfig) { - cfg.withValueTags = true - } -} diff --git a/internal/version/version.go b/internal/version/version.go index fe4fad9c4c..aeb0266f38 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -8,4 +8,4 @@ package version // Tag specifies the current release tag. It needs to be manually // updated. A test checks that the value of Tag never points to a // git tag that is older than HEAD. -const Tag = "v1.24.0" +const Tag = "v1.24.1"