Support Jaeger tracer #1030 by IKSIN · Pull Request #1147 · thanos-io/thanos

IKSIN · 2019-05-15T14:47:29Z

Changes

Refactoring tracing package.
Add support for Jaeger tracing.

Verification

test from pkg/tracing/tracing_test.go moved to pkg/tracing/provider/stackdriver/tracer_test.go
our Thanos fork already integrated with Jaeger
Run query on Google Compute Engine.

…tracing # Conflicts: # pkg/tracing/gct.go # pkg/tracing/provider/stackdriver/tracer_test.go # pkg/tracing/tracing.go

bwplotka

Thanks for this! It looks generally good, but I would really vote for sticking to the same configuration method we did for object storage.

Essentially factory like this: https://github.com/improbable-eng/thanos/blob/2c4f64b1b96907295a7f8e99d8fd64697f0eb12a/pkg/objstore/client/factory.go#L38
And using pathOrContent flag (: What do you think? (:

We are really grateful that you contribute back with the awesome addition like this 👍 Let us know what other awesome things you have in your fork 💪 (:

bwplotka · 2019-05-16T21:38:15Z

docs/components/bucket.md

                           GCP project to send Google Cloud Trace tracings to.
                           If empty, tracing will be disabled.
-      --gcloudtrace.sample-factor=1
+      --stackdriver.sample-factor=1


What do you think about having flags totally provider agnostic, same way we do for objconfig? Benefits would be great as everytime you change or add thousands of customization flags to one provider the flag help stays clean relevant for all users?
Even though we only agreed ont jaeger and stackdriver for now. What do you think?

I would stick to objstore config like configuration, mainly for consistency. Shouldn't be much more code

Also it would be great to move most of configuration to one file to have prometheus-like configuration approach. Its convenient to have all flags in one place.

I think it's a bigger decision. We chose not to do it as:

We have many smaller components instead of big single one like Prometheus

Most of options are not dynamic. Adding this dynamicity is enourmous amount of work

Objstore is separate to tracing. And it could be multiple of objstore configs as well, so it's hard to unifiy.

But someday (: Also, we are fans of flags (: for readability

Okay, got it, thanks! :)

bwplotka · 2019-05-16T21:41:40Z

pkg/tracing/provider/factory.go

+	return f
+}
+
+// Create creates the tracer in appliance with a tracerType


Missing trailing period

bwplotka · 2019-05-16T21:42:27Z

pkg/tracing/provider/factory.go

+
+// Create creates the tracer in appliance with a tracerType
+func (f *Factory) Create(ctx context.Context, logger log.Logger, serviceName string) (opentracing.Tracer, io.Closer) {
+	var tracer opentracing.Tracer


I would suggest this to make it bit more elegant

var ( )

Also TBH I would just name return arguments (:

bwplotka · 2019-05-16T21:50:53Z

pkg/tracing/provider/factory.go

+	var err error
+	factory, ok := f.factories[*f.tracingType];
+	if !ok {
+		level.Info(logger).Log("msg", "Invalid tracer type. Tracing will be disabled.", "err", err)


I am not fan of this. I would prefer error handling done outside of factory I think.

Reason is, that you should fail fast - if user actually provide some tracing configuration - if it's wrong you want to fail Thanos process immdiately (since it's on start - it's ok)

Allowing silent errors (only log lines and Info) is not enough - it will be missed and create annoyance. Can we fix this? (:

bwplotka · 2019-05-16T21:52:07Z

pkg/tracing/provider/jaeger/factory.go

+)
+
+// Factory implements tracing.Factory for Jaeger Tracer.
+type Factory struct {


Suggested change

type Factory struct {

type Factory struct {}

bwplotka · 2019-05-16T21:55:24Z

pkg/tracing/provider/jaeger/factory.go

+
+// Create implements tracing.Factory
+func (f *Factory) Create(ctx context.Context, logger log.Logger, serviceName string) (opentracing.Tracer, io.Closer, error) {
+	cfg, err := config.FromEnv()


Hmmm this is again - breaking consistency with other things we have. Having verbose config instead like objstore config would allow dynamic docs like here: https://thanos.io/storage.md/#aws-s3-configuration that is auto generated on every commit.

This way we are sure that all fields are known to the users for exact version of Thanos binary (: I know it's additional work, but IMO it would be awesome to allow specfying this not from env but from config (which is can be passed directly by flags as well). Additionally grabing from env might be possible as well

bwplotka · 2019-05-16T21:56:00Z

pkg/tracing/provider/jaeger/factory.go

+		JaegerDebugHeader: tracing.ForceTracingBaggageKey,
+	}
+	cfg.Headers.ApplyDefaults()
+	if serviceName != "" {


we would not need this if we would explicitly parse config from flags (:

bwplotka · 2019-05-16T21:56:30Z

pkg/tracing/provider/jaeger/factory.go

+		logger: logger,
+	}
+	jMetricsFactory := prometheus.New()
+	tracer, closer, err := cfg.NewTracer(


just return cfg.NewTracer(...) would be enough here?

bwplotka · 2019-05-16T21:56:36Z

pkg/tracing/provider/jaeger/factory.go

+	return tracer, closer, nil
+}
+
+// RegisterKingpinFlags implements tracing.Factory


bwplotka · 2019-05-16T21:57:29Z

pkg/tracing/provider/stackdriver/tracer.go

@@ -0,0 +1,112 @@
+package stackdriver


just curious, any change here or just move?

Also can we look on pjg/objstore package? Can we stick to same approach and just skip this provider directory? What do you think? (: I think both makes sense, just let's choose one

not addressed

IKSIN · 2019-05-17T08:51:57Z

@bwplotka Hello =) I sent this pull-request to discuss configurations and flags. I agree with configuration via the same configuration method we did for object storage, but I want to save jaeger.FromEnv configuration. We deploy prometheus with sidecar via prometheus-operator and configuration via ENV is most convenient.

bwplotka · 2019-05-17T09:16:26Z

@IKSIN this makes sense, and I am sure we can do proper consistent config from flag thing AND support doing it from env as well (:

See https://github.com/improbable-eng/thanos/blob/aedcb2e05b28f7fa35695be9c4127646dea5246e/pkg/objstore/s3/s3.go#L333

IKSIN · 2019-05-22T10:21:15Z

@bwplotka Hello) Method to configure tracing was changed as you want (jaeger config from file not implemented yet).
what are your suggestions to use configuration from ENV?

bwplotka

Some comments, generally that's what we want ❤️ Thanks for the good work!

bwplotka · 2019-05-22T13:02:51Z

cmd/thanos/flags.go

 }
+
+
+func regCommonTracingFlags(cmd *kingpin.Application, suffix string, required bool, extraDesc ...string) *pathOrContent {


let's remove suffix and require: YAGNI - we don't need that now (: same with extraDesc

bwplotka · 2019-05-22T13:03:45Z

cmd/thanos/main.go

-		String()
-	gcloudTraceSampleFactor := app.Flag("gcloudtrace.sample-factor", "How often we send traces (1/<sample-factor>). If 0 no trace will be sent periodically, unless forced by baggage item. See `pkg/tracing/tracing.go` for details.").
-		Default("1").Uint64()
+	//tracingFactory := provider.NewFactory(app)


Let's remove commented code from PR if it's rdy for review (:

bwplotka · 2019-05-22T13:05:23Z

cmd/thanos/main.go

+		var confContentYaml []byte
+		confContentYaml, err = tracingConfig.Content()
+		//tracer, closer = tracingFactory.Create(ctx, logger, *debugName)
+		tracer, closer, err = provider.NewTracer(ctx, logger, confContentYaml)


We need to manually make sure closer is closed on error cases of this function

pkg/tracing/provider/factory.go

bwplotka · 2019-05-22T13:06:45Z

pkg/tracing/provider/factory.go

+	level.Info(logger).Log("msg", "loading tracing configuration")
+	tracingConf := &TracingConfig{}
+	if err := yaml.UnmarshalStrict(confContentYaml, tracingConf); err != nil {
+		return &opentracing.NoopTracer{}, ioutil.NopCloser(nil), errors.Wrap(err, "parsing config YAML file")


Suggested change

return &opentracing.NoopTracer{}, ioutil.NopCloser(nil), errors.Wrap(err, "parsing config YAML file")

return &opentracing.NoopTracer{}, ioutil.NopCloser(nil), errors.Wrap(err, "parsing config tracing YAML")

bwplotka · 2019-05-22T13:08:21Z

pkg/tracing/provider/jaeger/jaeger.go

+}
+
+func parseConfig(conf []byte) (Config, error) {
+	config := Config{}


Too shallow function IMO - it does not hide much. What do you think about inlining this?

bwplotka · 2019-05-22T13:09:20Z

pkg/tracing/provider/jaeger/jaeger.go

+}
+
+func NewTracerWithConfig(ctx context.Context, logger log.Logger, conf Config) (opentracing.Tracer, io.Closer, error) {
+	cfg, err := config.FromEnv()


We should grab from env only if config is empty? So Flags > Environment? That avoids confusion

bwplotka · 2019-05-22T13:10:00Z

pkg/tracing/provider/jaeger/jaeger.go

+		logger: logger,
+	}
+	jMetricsFactory := prometheus.New()
+	tracer, closer, err := cfg.NewTracer(


literally return cfg.NewTracer(.... is enough here (:

pkg/tracing/provider/stackdriver/stackdriver.go

bwplotka · 2019-05-22T13:10:19Z

pkg/tracing/provider/stackdriver/stackdriver.go

+	if err != nil {
+		return nil, nil, err
+	}
+	println(config.ProjectId)


Can we kill this?

bwplotka · 2019-05-30T13:16:34Z

cmd/thanos/main.go

 		}, func(error) {
-			if err := closeFn(); err != nil {
-				level.Warn(logger).Log("msg", "closing tracer failed", "err", err)
+			if closer != nil {


closer is never nil in this case, right?

bwplotka

It LGTM! Thanks for this! We need changelog entry, but we can add it ourselve as well.

povilasv · 2019-05-30T13:56:52Z

pkg/tracing/jaeger/jaeger.go

+	)
+	if conf != nil {
+		level.Info(logger).Log("msg", "loading Jaeger tracing configuration from YAML")
+		cfg, err = FromYaml(conf)


Suggested change

cfg, err = FromYaml(conf)

cfg, err = ParseConfigFromYaml(conf)

pkg/tracing/jaeger/jaeger.go

pkg/tracing/stackdriver/stackdriver.go

povilasv · 2019-05-30T14:02:24Z

pkg/tracing/jaeger/config_yaml.go

+	}
+
+	if e := conf.RPCMetrics; e != "" {
+		if value, err := strconv.ParseBool(e); err == nil {


Couldn't we make a field bool and get rid of parsing?

Good point, missed that - We can enforce YAML unmarshalling to parse that for us.

povilasv · 2019-05-30T14:02:35Z

pkg/tracing/jaeger/config_yaml.go

+	}
+
+	if e := os.Getenv(conf.Disabled); e != "" {
+		if value, err := strconv.ParseBool(e); err == nil {


povilasv · 2019-05-30T14:04:57Z

pkg/tracing/jaeger/config_yaml.go

+	}
+
+	if e := cfg.SamplerParam; e != "" {
+		if value, err := strconv.ParseFloat(e, 64); err == nil {


Shouldn't Config.SamplerParam be float64 in struct?

povilasv · 2019-05-30T14:05:04Z

pkg/tracing/jaeger/config_yaml.go

+	}
+
+	if e := cfg.SamplerMaxOperations; e != "" {
+		if value, err := strconv.ParseInt(e, 10, 0); err == nil {


povilasv · 2019-05-30T14:05:11Z

pkg/tracing/jaeger/config_yaml.go

+	}
+
+	if e := cfg.SamplerRefreshInterval; e != "" {
+		if value, err := time.ParseDuration(e); err == nil {


bwplotka

One blocker @povilasv found and LGTM

bwplotka · 2019-05-30T14:08:52Z

pkg/tracing/jaeger/config_yaml.go

+	}
+
+	if e := conf.RPCMetrics; e != "" {
+		if value, err := strconv.ParseBool(e); err == nil {


Good point, missed that - We can enforce YAML unmarshalling to parse that for us.

povilasv

I think we should simplify Config struct, instead of having all the fields as string and parsing that, let's just let the yaml unmarshaller to do the work.

# Conflicts: # go.mod # go.sum

IKSIN · 2019-06-03T15:04:54Z

@bwplotka Done.

bwplotka

Thanks for awesome work on this. LGTM

There are some minior comment nits to be strict... ((:

bwplotka · 2019-06-04T10:09:59Z

pkg/tracing/jaeger/config_yaml.go

+	Tags                   string `yaml:"tags"`
+	SamplerType            string `yaml:"sampler_type"`
+	SamplerParam           float64 `yaml:"sampler_param"`
+	SamplerManagerHostPort string `yaml:"sampler_manager_host_port"`


Some docs on this structure would be nice at some point in next PR (:

bwplotka · 2019-06-04T10:10:36Z

pkg/tracing/jaeger/jaeger.go

+)
+
+
+// Tracer extends opentracing.Tracer


missing trailig period and all below. (:

bwplotka · 2019-06-04T10:11:02Z

pkg/tracing/stackdriver/stackdriver.go

+	"gopkg.in/yaml.v2"
+)
+
+// Config - YAML configuration


missing trailing period on comment.

bwplotka · 2019-06-04T10:11:08Z

pkg/tracing/stackdriver/stackdriver.go

+	SampleFactor uint64 `yaml:"sample_factor"`
+}
+
+// NewTracer create tracer from YAML


this and below

addressed (:

IKSIN · 2019-06-04T16:09:24Z

Refactored )

bwplotka · 2019-06-04T16:43:52Z

Awesome! Thank you for awesome job on this! we have Jaeger support now 🎉

d-ulyanov · 2019-06-04T18:22:30Z

Great job, guys!
And thanks a lot for thorough review. We'll add more spans to code cuz currently there are a lot of blind spots. I think we'll share this code soon too :)

bwplotka · 2019-06-04T19:37:47Z

Yes, we can definitely do better in terms of spans. Also we can stream better in Querier proxy.go code, so we can tell more on what is the slowest StoreAPI etc. (: If you can put a github issue if there is not any on particular parts that's needs attention, someone can grab it as well (:

…

On Tue, 4 Jun 2019 at 19:22, Dmitry Ulyanov ***@***.***> wrote: Great job, guys! And thanks a lot for thorough review. We'll add more spans to code cuz currently there are a lot of blind spots. I think we'll share this code soon too :) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1147>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVA3O7ZYJJYFZVWAPKT7ITPY2XGVANCNFSM4HNECJQQ> .

krasi-georgiev · 2019-06-05T12:46:34Z

pkg/tracing/stackdriver/tracer.go

+		return nil, nil, err
+	}
+
+	r, err := gcloudtracer.NewRecorder(


not sure how the tests passed her as gcloudtracer is not defined anywhere and when I run make locally it fails there.

did you download the required go mods?

yep I found the culprit the module was corrupted during the download.

it was missleading that the module name is gcloudtracer and the import path is github.com/lovoo/gcloud-opentracing, strange why they did it this way.

* Add jaeger tracing feature. * Remove comments * Remove comments * Refactoring tracing * Implementing jaeger logger. * Add jaeger force tracing header. * Use debugName for tracing service-name * RemoveFactory config * Formatting fix; Use io.Closer intead func() error * Rename gcloud => stackdriver * Refactoring google tracing testing. * Delete comments. * Rename gcloud flags => stackdriver. * Update tracing docs. * Fix ..traceType to ..tracerType * Refactoring NoopTracer; Comments for exported functions. * Remove noop tracer. Fix docs. * Remove noop tracer. Fix docs. * Config tracing same as objstore. * Configure jaeger tracing from YAML. Some small fixes. * Fix errcheck * Add X-Thanos-Trace-Id HTTP header for simplified search traces * Cleanup * make docs * Add store addr to tracing tags. * Tracing refactoring. * Fix noop tracing closer. * Add few tracing spans. * Pass prometheus registry to jaeger. * go.mod * Refactoring * Resolve go mod conflicts * Remove comments. * PR refactoring for review. * Format files.

Aleksey Sin added 18 commits May 8, 2019 18:46

Add jaeger tracing feature.

0f2a463

Remove comments

0e3f2eb

Remove comments

39e9bcb

Refactoring tracing

304f4da

Implementing jaeger logger.

bb5fa34

Add jaeger force tracing header.

a97415a

Use debugName for tracing service-name

38d252c

RemoveFactory config

4c03137

Formatting fix; Use io.Closer intead func() error

3869ce4

Rename gcloud => stackdriver

cede9a4

Refactoring google tracing testing.

27f3ef0

Delete comments.

3d12afe

Rename gcloud flags => stackdriver.

d24e376

Update tracing docs.

eed06c6

Fix ..traceType to ..tracerType

eb405ce

Refactoring NoopTracer; Comments for exported functions.

c745171

Remove noop tracer. Fix docs.

331a929

Remove noop tracer. Fix docs.

47f3fd1

IKSIN marked this pull request as ready for review May 15, 2019 14:47

IKSIN closed this May 15, 2019

Merge remote-tracking branch 'improbable/master' into feature/jaeger-…

ff1b837

…tracing # Conflicts: # pkg/tracing/gct.go # pkg/tracing/provider/stackdriver/tracer_test.go # pkg/tracing/tracing.go

IKSIN reopened this May 15, 2019

IKSIN mentioned this pull request May 15, 2019

Support Jaeger tracer #1030

Closed

bwplotka reviewed May 16, 2019

View reviewed changes

Config tracing same as objstore.

ff5a2aa

bwplotka reviewed May 22, 2019

View reviewed changes

Configure jaeger tracing from YAML. Some small fixes.

2d74602

bwplotka reviewed May 30, 2019

View reviewed changes

bwplotka approved these changes May 30, 2019

View reviewed changes

povilasv reviewed May 30, 2019

View reviewed changes

bwplotka reviewed May 30, 2019

View reviewed changes

povilasv previously requested changes May 30, 2019

View reviewed changes

Aleksey Sin added 5 commits May 31, 2019 10:54

Merge branch 'imaster' into feature/jaeger-tracing

9ced678

# Conflicts: # go.mod # go.sum

Refactoring

72d5559

Merge branch 'imaster' into feature/jaeger-tracing

ff15def

# Conflicts: # go.mod # go.sum

Resolve go mod conflicts

94aabb3

Remove comments.

4e98096

brancz requested a review from povilasv June 4, 2019 10:06

bwplotka approved these changes Jun 4, 2019

View reviewed changes

Aleksey Sin added 3 commits June 4, 2019 18:21

Merge branch 'imaster' into feature/jaeger-tracing

6b9f48f

PR refactoring for review.

b70f05a

Format files.

847cc91

bwplotka merged commit fa9ca8b into thanos-io:master Jun 4, 2019

krasi-georgiev reviewed Jun 5, 2019

View reviewed changes

sudeepknair mentioned this pull request Jul 19, 2019

Jaeger integration with Thanos #1339

Closed

		}


		func regCommonTracingFlags(cmd kingpin.Application, suffix string, required bool, extraDesc ...string) pathOrContent {

	return &opentracing.NoopTracer{}, ioutil.NopCloser(nil), errors.Wrap(err, "parsing config YAML file")
	return &opentracing.NoopTracer{}, ioutil.NopCloser(nil), errors.Wrap(err, "parsing config tracing YAML")

	cfg, err = FromYaml(conf)
	cfg, err = ParseConfigFromYaml(conf)

Conversation

IKSIN commented May 15, 2019

Changes

Verification

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IKSIN commented May 17, 2019

Uh oh!

bwplotka commented May 17, 2019

Uh oh!

IKSIN commented May 22, 2019

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

bwplotka May 17, 2019 •

edited

Loading

bwplotka May 16, 2019 •

edited

Loading