.*: Add support for tracing with Lightstep #1678

antonio · 2019-10-23T13:27:18Z

This patch adds support to use Lightstep as a tracing provider.

The implementation follows the Lightstep client library documentation, but does not set an event handler to log events as it was incredibly noisy and not really useful, making it impossible to follow what was going on in the logs. The output was similar to lines below, with a new line every 2-3 seconds.

level=info ts=2019-10-23T11:14:38.16467172Z caller=lightstep.go:55 msg="STATUS REPORT start: 2019-10-23 04:14:35.636670112 -0700 PDT m=+455.516288412, end: 2019-10-23 04:14:38.136701963 -0700 PDT m=+458.016320258, dropped spans: 0, encoding errors: 0" 
level=info ts=2019-10-23T11:14:40.66495401Z caller=lightstep.go:55 msg="STATUS REPORT start: 2019-10-23 04:14:38.136701963 -0700 PDT m=+458.016320258, end: 2019-10-23 04:14:40.636668107 -0700 PDT m=+460.516286403, dropped spans: 0, encoding errors: 0" 
level=info ts=2019-10-23T11:14:43.165782205Z caller=lightstep.go:55 msg="STATUS REPORT start: 2019-10-23 04:14:40.636668107 -0700 PDT m=+460.516286403, end: 2019-10-23 04:14:43.136700344 -0700 PDT m=+463.016318639, dropped spans: 0, encoding errors: 0" 
level=info ts=2019-10-23T11:14:45.664835433Z caller=lightstep.go:55 msg="STATUS REPORT start: 2019-10-23 04:14:43.136700344 -0700 PDT m=+463.016318639, end: 2019-10-23 04:14:45.636658483 -0700 PDT m=+465.516276779, dropped spans: 0, encoding errors: 0"

Lightstep allows setting up many more options when instantiating the tracer, but I decided to keep it simple for now and only make it possible to set the access token (required) and the address of the collector.

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Add Lightstep as a tracing provider.

Verification

I've tested it against our on-prem installation.

pkg/tracing/lightstep/lightstep.go

bwplotka

Nice! LGTM, small nits only (:

docs/tracing.md

pkg/tracing/lightstep/lightstep.go

bwplotka · 2019-10-24T11:08:51Z

pkg/tracing/lightstep/lightstep.go

+
+	// Collector is the host, port, and plaintext option to use
+	// for the collector.
+	Collector lightstep.Endpoint `yaml:"collector"`


reusing config 👍 nice!

pkg/tracing/lightstep/lightstep.go

Signed-off-by: Antonio Santos <[email protected]>

Co-Authored-By: Bartlomiej Plotka <[email protected]> Signed-off-by: Antonio Santos <[email protected]>

Co-Authored-By: Povilas Versockas <[email protected]> Signed-off-by: Antonio Santos <[email protected]>

antonio · 2019-10-25T16:00:04Z

Please hold off on merging this one for now: I'm not sure it's related, but I'm experiencing issues when trying to kill the program with Ctrl-C. I'll investigate it further on Monday.

antonio · 2019-10-28T10:42:30Z

Please hold off on merging this one for now: I'm not sure it's related, but I'm experiencing issues when trying to kill the program with Ctrl-C. I'll investigate it further on Monday.

This is unrelated to this PR. I paired with a coworker on it today and he submitted a new PR fixing the issue.

bwplotka · 2019-10-28T16:38:17Z

Nice one, thanks. My changed was added as well: lightstep/lightstep-tracer-go#228
Can we upgrade this dep?

antonio · 2019-10-29T10:58:26Z

Can we upgrade this dep?

Sure, but they haven't released a new version yet as far as I can tell.

GiedriusS · 2019-10-29T21:12:41Z

Can we upgrade this dep?

Sure, but they haven't released a new version yet as far as I can tell.

Do they have any release cadence?

antonio · 2019-10-30T06:15:12Z

Do they have any release cadence?

From what I can see in the git tags, no. I've asked them in lightstep/lightstep-tracer-go#228 (comment)

antonio · 2019-11-06T05:39:28Z

A heads up that I'm still waiting for Lightstep to release a new version that includes @bwplotka's changes. Alternatively, we could merge this as it is and update it once the updated library is out.

GiedriusS · 2019-11-16T18:55:07Z

@antonio seems like it was released. Care to update this PR so that we could add support for Lightstep tracing in Thanos? (:

Signed-off-by: Antonio Santos <[email protected]>

lightstep/lightstep-tracer-go#228 introduced a new method to create a Tracer that returns an error when it does not succeed, instead of nil Signed-off-by: Antonio Santos <[email protected]>

antonio · 2019-11-18T15:51:37Z

Care to update this PR so that we could add support for Lightstep tracing in Thanos? (:

Done!

Signed-off-by: Antonio Santos <[email protected]>

GiedriusS

Awesome! Thanks for the good work.

@bwplotka

* Add support for tracing with Lightstep Signed-off-by: Antonio Santos <[email protected]> * Remove type cast Signed-off-by: Antonio Santos <[email protected]> * Add documentation for the exported methods and types Signed-off-by: Antonio Santos <[email protected]> * Apply suggestions from @bwplotka Co-Authored-By: Bartlomiej Plotka <[email protected]> Signed-off-by: Antonio Santos <[email protected]> * Inline variable Co-Authored-By: Povilas Versockas <[email protected]> Signed-off-by: Antonio Santos <[email protected]> * Update lightstep library and add proper error handling lightstep/lightstep-tracer-go#228 introduced a new method to create a Tracer that returns an error when it does not succeed, instead of nil Signed-off-by: Antonio Santos <[email protected]> * Remove unused import Signed-off-by: Antonio Santos <[email protected]> Signed-off-by: Aleksey Sin <[email protected]>

@bwplotka

* Add support for tracing with Lightstep Signed-off-by: Antonio Santos <[email protected]> * Remove type cast Signed-off-by: Antonio Santos <[email protected]> * Add documentation for the exported methods and types Signed-off-by: Antonio Santos <[email protected]> * Apply suggestions from @bwplotka Co-Authored-By: Bartlomiej Plotka <[email protected]> Signed-off-by: Antonio Santos <[email protected]> * Inline variable Co-Authored-By: Povilas Versockas <[email protected]> Signed-off-by: Antonio Santos <[email protected]> * Update lightstep library and add proper error handling lightstep/lightstep-tracer-go#228 introduced a new method to create a Tracer that returns an error when it does not succeed, instead of nil Signed-off-by: Antonio Santos <[email protected]> * Remove unused import Signed-off-by: Antonio Santos <[email protected]>

povilasv requested changes Oct 23, 2019

View reviewed changes

pkg/tracing/lightstep/lightstep.go Show resolved Hide resolved

pkg/tracing/lightstep/lightstep.go Outdated Show resolved Hide resolved

pkg/tracing/lightstep/lightstep.go Outdated Show resolved Hide resolved

povilasv approved these changes Oct 23, 2019

View reviewed changes

pkg/tracing/lightstep/lightstep.go Outdated Show resolved Hide resolved

pkg/tracing/lightstep/lightstep.go Outdated Show resolved Hide resolved

antonio force-pushed the lightstep-tracing branch from b10a383 to dc5b4ee Compare October 23, 2019 14:52

bwplotka approved these changes Oct 24, 2019

View reviewed changes

antonio and others added 5 commits October 25, 2019 08:23

Add support for tracing with Lightstep

401ee02

Signed-off-by: Antonio Santos <[email protected]>

Remove type cast

7e6b650

Signed-off-by: Antonio Santos <[email protected]>

Add documentation for the exported methods and types

4fd596a

Signed-off-by: Antonio Santos <[email protected]>

Apply suggestions from @bwplotka

1001596

Co-Authored-By: Bartlomiej Plotka <[email protected]> Signed-off-by: Antonio Santos <[email protected]>

Inline variable

78bdee0

Co-Authored-By: Povilas Versockas <[email protected]> Signed-off-by: Antonio Santos <[email protected]>

antonio force-pushed the lightstep-tracing branch from 8cd8a07 to 78bdee0 Compare October 25, 2019 06:27

Merge branch 'master' into lightstep-tracing

b3ed2d1

antonio added 2 commits November 18, 2019 16:41

Merge remote-tracking branch 'origin/master' into lightstep-tracing

249c317

Signed-off-by: Antonio Santos <[email protected]>

Update lightstep library and add proper error handling

d9acf6a

lightstep/lightstep-tracer-go#228 introduced a new method to create a Tracer that returns an error when it does not succeed, instead of nil Signed-off-by: Antonio Santos <[email protected]>

Remove unused import

4c44f3c

Signed-off-by: Antonio Santos <[email protected]>

GiedriusS approved these changes Nov 19, 2019

View reviewed changes

GiedriusS merged commit f840e18 into thanos-io:master Nov 19, 2019

antonio deleted the lightstep-tracing branch December 17, 2019 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.*: Add support for tracing with Lightstep #1678

.*: Add support for tracing with Lightstep #1678

antonio commented Oct 23, 2019 •

edited

Loading

bwplotka left a comment

bwplotka Oct 24, 2019

antonio Oct 25, 2019

antonio commented Oct 25, 2019

antonio commented Oct 28, 2019

bwplotka commented Oct 28, 2019

antonio commented Oct 29, 2019

GiedriusS commented Oct 29, 2019

antonio commented Oct 30, 2019

antonio commented Nov 6, 2019

GiedriusS commented Nov 16, 2019

antonio commented Nov 18, 2019

GiedriusS left a comment

.*: Add support for tracing with Lightstep #1678

.*: Add support for tracing with Lightstep #1678

Conversation

antonio commented Oct 23, 2019 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Oct 24, 2019

Choose a reason for hiding this comment

antonio Oct 25, 2019

Choose a reason for hiding this comment

antonio commented Oct 25, 2019

antonio commented Oct 28, 2019

bwplotka commented Oct 28, 2019

antonio commented Oct 29, 2019

GiedriusS commented Oct 29, 2019

antonio commented Oct 30, 2019

antonio commented Nov 6, 2019

GiedriusS commented Nov 16, 2019

antonio commented Nov 18, 2019

GiedriusS left a comment

Choose a reason for hiding this comment

antonio commented Oct 23, 2019 •

edited

Loading