Skip to content
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

Added Tag Configuration Support Lightstep Tracing #4175

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ We use _breaking :warning:_ to mark changes that are not backward compatible (re

- [#4107](https://github.com/thanos-io/thanos/pull/4107) Store: `LabelNames` and `LabelValues` now support label matchers.
- [#4171](https://github.com/thanos-io/thanos/pull/4171) Docker: Busybox image updated to latest (1.33.1)
- [#4175](https://github.com/thanos-io/thanos/pull/4175) Added Tag Configuration Support Lightstep Tracing
- [#4176](https://github.com/thanos-io/thanos/pull/4176) Query API: Adds optional `Stats param` to return stats for query APIs

### Fixed
-
### Changed
Expand Down
1 change: 1 addition & 0 deletions docs/tracing.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,5 @@ config:
port: 0
plaintext: false
custom_ca_cert_file: ""
tags: ""
```
38 changes: 38 additions & 0 deletions pkg/tracing/lightstep/lightstep.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ package lightstep
import (
"context"
"io"
"os"
"strings"

"github.com/lightstep/lightstep-tracer-go"
"github.com/opentracing/opentracing-go"
Expand All @@ -21,6 +23,9 @@ type Config struct {
// Collector is the host, port, and plaintext option to use
// for the collector.
Collector lightstep.Endpoint `yaml:"collector"`

// Tags is a string comma-delimited of key value pairs that holds metadata that will be sent to lightstep
Tags string `yaml:"tags"`
}

// Tracer wraps the Lightstep tracer and the context.
Expand All @@ -43,9 +48,15 @@ func NewTracer(ctx context.Context, yamlConfig []byte) (opentracing.Tracer, io.C
return nil, nil, err
}

var tags opentracing.Tags
if config.Tags != "" {
tags = parseTags(config.Tags)
}

options := lightstep.Options{
AccessToken: config.AccessToken,
Collector: config.Collector,
Tags: tags,
}
lighstepTracer, err := lightstep.CreateTracer(options)
if err != nil {
Expand All @@ -58,3 +69,30 @@ func NewTracer(ctx context.Context, yamlConfig []byte) (opentracing.Tracer, io.C
}
return t, t, nil
}

// parseTags parses the given string into a map of strings to empty interface.
// Spec for this value:
// - comma separated list of key=value
// - value can be specified using the notation ${envVar:defaultValue}, where `envVar`
// is an environment variable and `defaultValue` is the value to use in case the env var is not set.
func parseTags(sTags string) opentracing.Tags {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a quick unit-test for it? (:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that 😜 But there aren't any other unit tests in the tracing package (except stackdriver), so I figured it didn't make sense to break that ground for this simple change.

Should we add in a testing file and scaffolding for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch all that... I went ahead and added testing. PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haaa, yea, something pragmatic would work, e.g just testing parsting tags (: Like what you did! Thanks!

pairs := strings.Split(sTags, ",")
tags := make(opentracing.Tags)
for _, p := range pairs {
kv := strings.SplitN(p, "=", 2)
k, v := strings.TrimSpace(kv[0]), strings.TrimSpace(kv[1])

if strings.HasPrefix(v, "${") && strings.HasSuffix(v, "}") {
ed := strings.SplitN(v[2:len(v)-1], ":", 2)
e, d := ed[0], ed[1]
v = os.Getenv(e)
if v == "" && d != "" {
v = d
}
}

tags[k] = v
}

return tags
}
60 changes: 60 additions & 0 deletions pkg/tracing/lightstep/lightstep_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright (c) The Thanos Authors.
// Licensed under the Apache License 2.0.

package lightstep

import (
"os"
"testing"

"github.com/thanos-io/thanos/pkg/testutil"

"github.com/opentracing/opentracing-go"
)

func TestParseTags(t *testing.T) {
type testData struct {
input string
description string
expectedOutput opentracing.Tags
}

testingData := []testData{
{
description: `A very simple case, key "foo" and value "bar"`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing 🤗

input: "foo=bar",
expectedOutput: opentracing.Tags{"foo": "bar"},
},
{
description: `A simple case multiple keys, keys ["foo", "bar"] and values ["foo", "bar"]`,
input: "foo=foo,bar=bar",
expectedOutput: opentracing.Tags{"foo": "foo", "bar": "bar"},
},
{
description: `A case with empty environment variable, key "foo" and value ""`,
input: "foo=${TEST:}",
expectedOutput: opentracing.Tags{"foo": ""},
},
{
description: `A case with empty environment variable, key "foo" and value ""`,
input: "foo=${TEST:}",
expectedOutput: opentracing.Tags{"foo": ""},
},
{
description: `A case with default environment variable, key "foo" and value "default"`,
input: "foo=${TEST:default}",
expectedOutput: opentracing.Tags{"foo": "default"},
},
{
description: `A case with real environment variable, key "foo" and value "env-bar"`,
input: "foo=${_TEST_PARSE_TAGS:default}",
expectedOutput: opentracing.Tags{"foo": "env-bar"},
},
}

os.Setenv("_TEST_PARSE_TAGS", "env-bar")
for _, test := range testingData {
t.Logf("testing %s\n", test.description)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a biggy, but we can use t.Run for consistency instead this (: But that's fine for now, for this PR, just let's remember for next one

testutil.Equals(t, test.expectedOutput, parseTags(test.input))
}
}