Skip to content

Commit

Permalink
store/proxy: fix label values span (#6795)
Browse files Browse the repository at this point in the history 
Each tracing.StartSpan() writes a value into the given context so
there's a race if we keep reusing the same context. Fix this by starting
a new span in each goroutine. This also makes logical sense. Fixes the
following race:

```
15:21:13 querier-1: WARNING: DATA RACE
15:21:13 querier-1: Read at 0x00c0009c5050 by goroutine 328:
15:21:13 querier-1: context.(*valueCtx).Value()
15:21:13 querier-1: /usr/local/go/src/context/context.go:751 +0x76
15:21:13 querier-1: github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/tracing.newClientSpanFromContext()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/v2@v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7/interceptors/tracing/client.go:87 +0x241
15:21:13 querier-1: github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/tracing.(*opentracingClientReportable).ClientReporter()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/v2@v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7/interceptors/tracing/client.go:51 +0x195
15:21:13 querier-1: github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/tracing.UnaryClientInterceptor.UnaryClientInterceptor.func1()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/v2@v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7/interceptors/client.go:19 +0x1a9
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/extgrpc.StoreClientGRPCOpts.ChainUnaryClient.func4.1.1()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/v2@v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7/chain.go:74 +0x10a
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/extgrpc.StoreClientGRPCOpts.(*ClientMetrics).UnaryClientInterceptor.func3()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-prometheus@v1.2.0/client_metrics.go:112 +0x126
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/extgrpc.StoreClientGRPCOpts.ChainUnaryClient.func4.1.1()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/v2@v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7/chain.go:74 +0x10a
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/extgrpc.StoreClientGRPCOpts.ChainUnaryClient.func4()
15:21:13 querier-1: /go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/v2@v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7/chain.go:83 +0x17b
15:21:13 querier-1: google.golang.org/grpc.(*ClientConn).Invoke()
15:21:13 querier-1: /go/pkg/mod/google.golang.org/grpc@v1.45.0/call.go:35 +0x25d
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/store/storepb.(*storeClient).LabelValues()
15:21:13 querier-1: /go/src/github.com/thanos-io/thanos/pkg/store/storepb/rpc.pb.go:1034 +0xe5
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/query.(*endpointRef).LabelValues()
15:21:13 querier-1: <autogenerated>:1 +0xa1                                                                                                                                        15:21:13 querier-1: github.com/thanos-io/thanos/pkg/store.(*ProxyStore).LabelValues.func1()
15:21:13 querier-1: /go/src/github.com/thanos-io/thanos/pkg/store/proxy.go:586 +0x323
15:21:13 querier-1: golang.org/x/sync/errgroup.(*Group).Go.func1()
15:21:13 querier-1: /go/pkg/mod/golang.org/x/sync@v0.3.0/errgroup/errgroup.go:75 +0x76
15:21:13 querier-1: Previous write at 0x00c0009c5050 by goroutine 325:
15:21:13 querier-1: context.WithValue()
15:21:13 querier-1: /usr/local/go/src/context/context.go:718 +0xce
15:21:13 querier-1: github.com/opentracing/opentracing-go.ContextWithSpan()
15:21:13 querier-1: /go/pkg/mod/github.com/opentracing/opentracing-go@v1.2.0/gocontext.go:17 +0xec
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/tracing.StartSpan()
15:21:13 querier-1: /go/src/github.com/thanos-io/thanos/pkg/tracing/tracing.go:73 +0x238
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/store.(*ProxyStore).LabelValues()
15:21:13 querier-1: /go/src/github.com/thanos-io/thanos/pkg/store/proxy.go:567 +0xb25
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/query.(*querier).LabelValues()
15:21:13 querier-1: /go/src/github.com/thanos-io/thanos/pkg/query/querier.go:422 +0x3f5
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/api/query.(*QueryAPI).labelValues()
15:21:13 querier-1: /go/src/github.com/thanos-io/thanos/pkg/api/query/v1.go:1092 +0x17d1
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/api/query.(*QueryAPI).labelValues-fm()
15:21:13 querier-1: <autogenerated>:1 +0x45
15:21:13 querier-1: github.com/thanos-io/thanos/pkg/api/query.(*QueryAPI).Register.GetInstr.func1.1()
```

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
  • Loading branch information
@GiedriusS
GiedriusS authored Oct 12, 2023
1 parent 728bda9 commit 8ebf748
Showing 1 changed file with 8 additions and 9 deletions.
17 changes: 8 additions & 9 deletions pkg/store/proxy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (

"github.com/go-kit/log"
"github.com/go-kit/log/level"
"github.com/opentracing/opentracing-go"
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
Expand Down Expand Up @@ -540,7 +539,6 @@ func (s *ProxyStore) LabelValues(ctx context.Context, r *storepb.LabelValuesRequ
mtx sync.Mutex
g, gctx = errgroup.WithContext(ctx)
storeDebugMsgs []string
span opentracing.Span
)

// We may arrive here either via the promql engine
Expand All @@ -564,12 +562,6 @@ func (s *ProxyStore) LabelValues(ctx context.Context, r *storepb.LabelValuesRequ
if storeID == "" {
storeID = "Store Gateway"
}
span, gctx = tracing.StartSpan(gctx, "proxy.label_values", tracing.Tags{
"store.id": storeID,
"store.addr": storeAddr,
"store.is_local": isLocalStore,
})
defer span.Finish()

// We might be able to skip the store if its meta information indicates it cannot have series matching our query.
if ok, reason := storeMatches(gctx, st, s.debugLogging, r.Start, r.End); !ok {
Expand All @@ -583,7 +575,14 @@ func (s *ProxyStore) LabelValues(ctx context.Context, r *storepb.LabelValuesRequ
}

g.Go(func() error {
resp, err := st.LabelValues(gctx, &storepb.LabelValuesRequest{
span, spanCtx := tracing.StartSpan(gctx, "proxy.label_values", tracing.Tags{
"store.id": storeID,
"store.addr": storeAddr,
"store.is_local": isLocalStore,
})
defer span.Finish()

resp, err := st.LabelValues(spanCtx, &storepb.LabelValuesRequest{
Label: r.Label,
PartialResponseDisabled: r.PartialResponseDisabled,
Start: r.Start,
Expand Down

0 comments on commit 8ebf748

Please sign in to comment.