Use zipkin-go #123

jcchavezs · 2019-02-26T14:08:05Z

This PR aims to use zipkin-go library and turn this library into a bridge with opentracing rather than a native implementation. This provides multiple advantages, mostly the fact that v2 reporting will be supported and also that this bridge will be able to obtain all the benefits from the new features of zipkin-go.

Closes #125

Ping @stakhiv @basvanbeek @ONG-YA

codefromthecrypt · 2019-03-26T09:21:59Z

what is the status of this? it looks like there is no code on the branch. was that by accident?

Co-authored-by: José Carlos Cháve <[email protected]>

codefromthecrypt

wow so red!

ONG-YA · 2019-04-23T09:03:56Z

hope to see the release version soon

feat(zipkin_go): adds b3 propagation for http.

Tracer wrapping

jcchavezs · 2019-05-19T08:56:36Z

This is pretty functional. Couple of questions:

What is missing (besides updating the readme)?
Do we want to include the opentracing observers? v1 version did and I think some other ot instrumentation do.
Do we want to launch including the GRPC b3 propagation?
Any other concern?

Ping @adriancole @basvanbeek @stakhiv

basvanbeek · 2019-05-19T11:40:20Z

So it seems we have a couple of removed features moving from native to wrapper:

we don't support the deprecated logging methods that are still part of the OT interface (nice to have)
we don't support baggage (nice to have: could be implemented here instead the Zipkin go plans to support the more sensible white list approach)
we don't support observers (this was contributed in Monitoring platform events with zipkin-go and opentracing #50 from outside our org, unsure if still used)

So honestly I'm ok to merge without these items but they could pop up. I do think it would be better to launch with gRPC B3.

chore: enables build for go 1.8.

jcchavezs · 2019-05-19T12:34:11Z

we don't support the deprecated logging methods that are still part of the OT interface (nice to have)

Since they are deprecated I was thinking on waiting for someone to request for it? unless we see popular opentracing instrumentation using this? Otherwise I could add them.

we don't support baggage (nice to have: could be implemented here instead the Zipkin go plans to support the more sensible white list approach)

True, this one is probably needed.

we don't support observers (this was contributed in #50 from outside our org, unsure if still used)

Worth to ping @hkshaw1990 here (to see if they are still using it). On the other hand, I see an issue here. The optional arguments for the NewTracer are the zipkin-go ones which means that making new arguments will feel weird. One option is to not to have such a NewTracer and only a Wrap method, another option would be to wrap the functional arguments of zipkin-go to have consistency with the additional functional arguments on zipkin-go-opentracing. Any thoughts @basvanbeek ?

basvanbeek · 2019-05-19T12:58:06Z

There is quite some legacy code out there using the old log methods. My former company for instance...

Bas

jcchavezs · 2019-05-19T13:00:34Z

+1

…tions feat: adds tracer native options.

…_ease_migration chore(zipkin): adds sampler type to ease migration.

basvanbeek · 2019-08-02T14:07:23Z

i’ll try to look shortly...

fix: changes the default propagation format.

ONG-YA · 2019-08-07T06:43:40Z

I think this is ready to be merged so I know @basvanbeek is busy but it would be nice to get some feedback at some point :).

@ONG-YA feel free to try this branch and give us feedback.

i'll give it a try~

…library.

jcchavezs · 2019-08-07T09:24:50Z

go.mod

+go 1.12
+
+require (
+	github.com/gorilla/mux v1.7.3 // indirect


I believe this should not be here. It comes from the example.

jcchavezs · 2019-08-07T10:50:41Z

I am glad to say that we have this branch running in a production service.

codefromthecrypt

thanks for deleting so much. I quickly looked at things and commented on what jumped out.

codefromthecrypt · 2019-08-10T08:02:44Z

bench_test.go

-		b.Fatalf("missing traces: expected %d, got %d", b.N, r)
-	}
-}
+//func BenchmarkSpan_100BaggageItems(b *testing.B) {


intentional?

yes, we don't support this yet and I would wait until extra fields take place in zipkin go.

codefromthecrypt · 2019-08-10T08:02:57Z

LICENSE

+same "printed page" as the copyright notice for easier
+identification within third-party archives.
+
+Copyright 2017 The OpenZipkin Authors


No idea on this but I copied it from zipkin-go. Should we change it there? https://github.com/openzipkin/zipkin-go/blob/master/LICENSE#L189

codefromthecrypt · 2019-08-10T08:03:09Z

bench_test.go

-func BenchmarkInject_Binary_100BaggageItems(b *testing.B) {
-	benchmarkInject(b, opentracing.Binary, 100)
-}
+// func BenchmarkInject_Binary_Empty(b *testing.B) {


intentional?

we don't support binary yet. Let's reintroduce it when it is supported.

codefromthecrypt · 2019-08-10T08:03:18Z

bench_test.go

-func BenchmarkExtract_Binary_100BaggageItems(b *testing.B) {
-	benchmarkExtract(b, opentracing.Binary, 100)
-}
+// func BenchmarkExtract_Binary_Empty(b *testing.B) {


intentional?

examples/cli_with_2_services/svc2/cmd/main.go

codefromthecrypt · 2019-08-10T08:06:20Z

propagation/http/http_test.go

+	"github.com/openzipkin/zipkin-go/reporter/recorder"
+)
+
+func TestHTTPExtractFlagsOnly(t *testing.T) {


a lot of this feels like copy/paste. is there a way to only test what's relevant here (employing the code from zipkin-go and only addressing wrapping concerns)

unfortunately we reuse a very simple thing from zipkin-go in this package but makes sense to revisit the test list.

codefromthecrypt · 2019-08-10T08:07:23Z

recorder_test.go

 type CountingRecorder int32

-func (c *CountingRecorder) RecordSpan(r RawSpan) {
+func (c *CountingRecorder) Send(span model.SpanModel) {


previous name is less distracting unless somehow the word send is interesting here. the type is recorder right?

yes and we changed the name to fulfill the interface.

codefromthecrypt · 2019-08-10T08:08:57Z

span_test.go

+	assert.Equal(t, "x", spans[0].Name)
+	assert.Equal(t, 3, len(spans[0].Annotations))
+	assert.Equal(t, map[string]string{"tag": "value"}, spans[0].Tags)
+	assert.Equal(t, spans[0].Annotations[0].Value, "event:payload")


event: prefix is probably not helpful

codefromthecrypt · 2019-08-10T08:10:10Z

tracer_options.go

+	// to allow deterministic sampling decisions to be made across different nodes.
+	sampler zipkin.Sampler
+
+	// unsampledNoop turns potentially expensive operations on unsampled


why have this? it seems half of the sampled local thing...

not sure I understand the question. it is here because of compatibility but also because it is supported in zipkin-go unless you refer to the actual description.

actually why do we need to copy options from zipkin-go anyway? could you not have a funciton that takes a zipkin-go tracer (with options already applied) plus OT relevant options? It seems less maintenance.. or am I missing something?

or by compatibility do you mean that this option was here before so that's why. Anyway, I'll leave you to it. I think you know I find this api surprising especially to someone using OT when they can just use zipkin-go directly if worried about perf. I would be surprised if layered instrumentation do smart things routinely, that's all.

Right, now I understand the question :). So, this option was already here: https://github.com/openzipkin-contrib/zipkin-go-opentracing/blob/master/tracer.go#L36 I just made the translation into the respectively part in zipkin-go.

I have removed many options that were here (most of them related to logs) and did not match zipkin-go but those easily having a counter part I kept.

BTW there is a method for wrapping an existing tracer: https://github.com/openzipkin-contrib/zipkin-go-opentracing/pull/123/files#diff-46db78603208f38452c0333bec12ab2cR57

I don't have strong feelings on keeping it or removing it but since we don't add any logic for this but to translate into zipkin option I'd rather keep it for retrocompatibility purposes.

tracer_options.go

codefromthecrypt · 2019-08-10T23:36:35Z

I suppose I meant to say I'm surprised to see checking in of commented out code. Would it be better to delete it then check it back in when we have functionality? Or is it typical in go to comment out code?

codefromthecrypt · 2019-08-10T23:37:11Z

maybe change CountingRecorder to CountingSender instead?

…

On Sun, Aug 11, 2019 at 2:51 AM José Carlos Chávez ***@***.***> wrote: @jcchavezs commented on this pull request. ________________________________ In recorder_test.go: > type CountingRecorder int32 -func (c *CountingRecorder) RecordSpan(r RawSpan) { +func (c *CountingRecorder) Send(span model.SpanModel) { yes and we changed the name to fulfill the interface. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

codefromthecrypt · 2019-08-10T23:38:45Z

not sure I understand the question. it is here because of compatibility but also because it is supported in zipkin-go unless you refer to the actual description.

Seems an odd api, unless there's a good reason to also export it here, I'd leave it out as OT is already going to imply some wrapping above which will probably obviate some of the cycles this feel it might save

…translation Adds support for tag translation

jcchavezs · 2019-08-11T08:39:07Z

I suppose I meant to say I'm surprised to see checking in of commented out
code. Would it be better to delete it then check it back in when we have
functionality? Or is it typical in go to comment out code?

Right, it was just lazyness to not to need to dig into it when the functionality is back. I will remove it

maybe change CountingRecorder to CountingSender instead?

Makes sense!

…_for_tags chore: adds tag translations for tags.

… wrapping an existing zipkin tracer.

chore: removes the option to create a new tracer

refactoring of propagation handling

jcchavezs · 2019-08-15T20:58:23Z

@ONG-YA here you go: https://github.com/openzipkin-contrib/zipkin-go-opentracing/releases/tag/v0.4.0

disable travis temporary.

fe8162f

jcchavezs force-pushed the use_zipkin_go branch from 0a767e9 to fe8162f Compare February 26, 2019 14:09

codefromthecrypt mentioned this pull request Mar 10, 2019

Sampling logic error? x-b3-flags / x-b3-sampled logic question #104

Closed

codefromthecrypt mentioned this pull request Mar 22, 2019

when support zipkin v2 #125

Closed

Yurii Stakhiv added 2 commits April 12, 2019 13:05

First types implementation using openzipkin/zipkin-go

e743899

Co-authored-by: José Carlos Cháve <[email protected]>

Removed zipkin-endpoint.go

e244997

codefromthecrypt reviewed Apr 12, 2019

View reviewed changes

jcchavezs changed the title ~~[WIP] Use zipkin-go underneath~~ [WIP] Use zipkin-go Apr 12, 2019

jcchavezs and others added 9 commits April 25, 2019 18:04

feat(zipkin_go): adds b3 propagation for http.

3e31767

chore: improves casting by using only needed interfaces.

3eff981

Merge pull request #126 from openzipkin-contrib/adds_propagation

f4b1237

feat(zipkin_go): adds b3 propagation for http.

removes unnecessary files.

26fce87

feat(zipkin_go): adds support for tracer wrapping.

4f3003a

Fixed tests. Enabled CI checks. Enabled go modules.

a37a5e1

Removed duplicated 'make test' run

8fe5626

Merge pull request #127 from openzipkin-contrib/tracer_wrapping

7461562

Tracer wrapping

chore: enables build for go 1.8.

9f06c26

jcchavezs changed the title ~~[WIP] Use zipkin-go~~ Use zipkin-go May 19, 2019

Merge pull request #132 from openzipkin-contrib/reenables_go_1_8

4c5e201

chore: enables build for go 1.8.

jcchavezs mentioned this pull request Jun 30, 2019

what is the difference between zipkin v1 with v2? openzipkin/zipkin#2644

Closed

jcchavezs added 3 commits July 5, 2019 15:29

feat: anative tracer options.

b3e8329

chore: address @basvanbeek feedback.

2896fa0

Merge pull request #133 from openzipkin-contrib/adds_native_tracer_op…

db1effb

…tions feat: adds tracer native options.

Merge pull request #135 from openzipkin-contrib/adds_sampler_type_for…

f8c7816

…_ease_migration chore(zipkin): adds sampler type to ease migration.

jcchavezs added 3 commits August 6, 2019 16:04

fix: changes the default propagation format.

b2836ce

Merge pull request #136 from openzipkin-contrib/fixes_propagation_format

776137e

fix: changes the default propagation format.

chore: fixes tag translation from OT to zipkin.

1bccb7d

jcchavezs added 2 commits August 7, 2019 09:59

chore: removes golang.org/x/tools/cmd/cover pull as it is in the std …

3c5e408

…library.

fix: fixes go mod install in go 1.12.

94e8e9d

jcchavezs commented Aug 7, 2019

View reviewed changes

feat: adds support for samplers.

447f2dc

jcchavezs force-pushed the use_zipkin_go branch from 1c927e5 to 447f2dc Compare August 7, 2019 14:44

codefromthecrypt reviewed Aug 10, 2019

View reviewed changes

jcchavezs added 3 commits August 11, 2019 10:11

Merge pull request #138 from openzipkin-contrib/adds_support_for_tag_…

1d0047c

…translation Adds support for tag translation

chore: adds tag translations for tags.

6e6623c

tests: improves tests semantics.

7086f07

jcchavezs and others added 7 commits August 11, 2019 10:42

chore: improves naming of CountingRecorder and remove commented code.

7d16594

Merge pull request #139 from openzipkin-contrib/adds_trag_translation…

fcfbbbb

…_for_tags chore: adds tag translations for tags.

chore: use better tags on example.

bf72dc1

chore: removes the option to create a new tracer and instead focus on…

51eaa8d

… wrapping an existing zipkin tracer.

Merge pull request #140 from openzipkin-contrib/removes_new_tracer

96684ea

chore: removes the option to create a new tracer

refactoring of propagation handling

0694388

Merge pull request #141 from openzipkin-contrib/refactor_propagation

f852068

refactoring of propagation handling

basvanbeek merged commit bff5926 into master Aug 15, 2019

jcchavezs deleted the use_zipkin_go branch September 17, 2019 12:29

Use zipkin-go #123

Use zipkin-go #123

Conversation

jcchavezs commented Feb 26, 2019 • edited Loading

codefromthecrypt commented Mar 26, 2019

codefromthecrypt left a comment

Choose a reason for hiding this comment

ONG-YA commented Apr 23, 2019

jcchavezs commented May 19, 2019

basvanbeek commented May 19, 2019

jcchavezs commented May 19, 2019

basvanbeek commented May 19, 2019

jcchavezs commented May 19, 2019 via email • edited Loading

basvanbeek commented Aug 2, 2019

ONG-YA commented Aug 7, 2019

Choose a reason for hiding this comment

jcchavezs commented Aug 7, 2019

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

codefromthecrypt commented Aug 10, 2019 via email

codefromthecrypt commented Aug 10, 2019 via email

codefromthecrypt commented Aug 10, 2019 via email • edited by jcchavezs Loading

jcchavezs commented Aug 11, 2019

jcchavezs commented Aug 15, 2019

jcchavezs commented Feb 26, 2019 •

edited

Loading

jcchavezs commented May 19, 2019 via email •

edited

Loading

jcchavezs Aug 11, 2019 •

edited

Loading

codefromthecrypt commented Aug 10, 2019 via email •

edited by jcchavezs

Loading