Add Google Cloud Pub Sub reporter #142

javierviera · 2019-08-06T08:58:03Z

Add a new reporter for GCP streaming service.

The idea is to have pub sub subscriber as zipkin collector to process the spans

# Conflicts: # go.mod # go.sum

reporter/pubsub/pubsub.go

jcchavezs · 2019-08-06T09:34:41Z

reporter/pubsub/pubsub.go

+	}
+	if r.client == nil {
+		ctx := context.Background()
+		proj := os.Getenv("GOOGLE_CLOUD_PROJECT")


I have mixed feelings here. In one hand I usually don't like env vars support for libraries but I don't see a trivial way to not to have mutually exclusive options like client and google_cloud_project. Maybe the env var is good enough. Any thoughts @basvanbeek ?

reporter/pubsub/pubsub.go

jcchavezs · 2019-08-06T09:36:43Z

Nice start @javierviera. I left some comments.

reporter/gcppubsub/pubsub.go

reporter/gcppubsub/pubsub_test.go

coveralls · 2019-08-06T10:53:39Z

Coverage decreased (-2.2%) to 60.797% when pulling 5744523 on javierviera:master into b1a3538 on openzipkin:master.

reporter/gcppubsub/pubsub.go

reporter/gcppubsub/pubsub_test.go

reporter/gcppubsub/pubsub.go

codefromthecrypt · 2019-08-07T00:46:17Z

usually we don't add reporters for transports that have no collectors. this leads to an incomplete feature story and possibly code maintained for only one user or site. can you mention what you are doing on the other side?

…

On Tue, Aug 6, 2019, 6:58 PM Javier Viera ***@***.***> wrote: Add a new reporter for GCP streaming service. The idea is tho have pub sub subscriber as zipkin collector to process the spans ------------------------------ You can view, comment on, or merge this pull request online at: #142 Commit Summary - Add GCP pubsub reporter - Merge remote-tracking branch 'upstream/master' - Merge remote-tracking branch 'upstream/master' - Add Pubsub dependencies - Configure topic with env name File Changes - *M* .gitignore <https://github.com/openzipkin/zipkin-go/pull/142/files#diff-0> (1) - *M* README.md <https://github.com/openzipkin/zipkin-go/pull/142/files#diff-1> (3) - *M* go.mod <https://github.com/openzipkin/zipkin-go/pull/142/files#diff-2> (14) - *M* go.sum <https://github.com/openzipkin/zipkin-go/pull/142/files#diff-3> (71) - *A* reporter/pubsub/pubsub.go <https://github.com/openzipkin/zipkin-go/pull/142/files#diff-4> (109) - *A* reporter/pubsub/pubsub_test.go <https://github.com/openzipkin/zipkin-go/pull/142/files#diff-5> (81) Patch Links: - https://github.com/openzipkin/zipkin-go/pull/142.patch - https://github.com/openzipkin/zipkin-go/pull/142.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#142?email_source=notifications&email_token=AAAPVVY2D4SHQ5O7FK7PXMTQDE4JXA5CNFSM4IJUCARKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HDSKWZA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV2NWMZODQK4VFOEFLDQDE4JXANCNFSM4IJUCARA> .

javierviera · 2019-08-07T06:37:43Z

@adriancole we are evaluating 2 options... most probably we will add a new collector in https://github.com/javierviera/zipkin/tree/master/zipkin-collector but we are also evaluating create a private google cloud function in GCP which pull Google PubSub message and add it to zipkin by API (https://zipkin.io/zipkin-api/#/default/post_spans)

reporter/gcppubsub/pubsub.go

jcchavezs · 2019-08-08T11:20:06Z

reporter/gcppubsub/pubsub.go

+type ReporterOption func(c *Reporter)
+
+// Send send span to topic
+func (r *Reporter) Send(s model.SpanModel) {


I am curious about the tradeoff for reporting one span each time of piling up them up to a point and then do the reporting. I would prefer to not to do a http call on every span cc @basvanbeek

We can for symmetry reasons with other reporters (and maybe optimization of message encoding/decoding). For request/response calls it is not needed as pubsub handles sending underneath and batches.... see https://godoc.org/cloud.google.com/go/pubsub

codefromthecrypt · 2019-08-09T00:50:09Z

ok probably if adding a collector, forking (and raising a pull request) on zipkin-gcp could be a nice route. the cloud function idea is also neat. I think span consumption is a neat use case of lambda though there are some gotchas the main collector code handles. Assuming we are to merge this, let's make sure this accepts the literally same data format as other transports (list of json or proto encoded spans). If you have time, please add/update an issue on zipkin-gcp about the pub-sub so that this is less tail wagging dog

…

On Wed, Aug 7, 2019, 2:37 PM Javier Viera ***@***.***> wrote: usually we don't add reporters for transports that have no collectors. this leads to an incomplete feature story and possibly code maintained for only one user or site. can you mention what you are doing on the other side? … <#m_3963051830818073207_m_6748561530216320011_m_-2186552932525891970_> On Tue, Aug 6, 2019, 6:58 PM Javier Viera *@*.***> wrote: Add a new reporter for GCP streaming service. The idea is tho have pub sub subscriber as zipkin collector to process the spans ------------------------------ You can view, comment on, or merge this pull request online at: #142 <#142> Commit Summary - Add GCP pubsub reporter - Merge remote-tracking branch 'upstream/master' - Merge remote-tracking branch 'upstream/master' - Add Pubsub dependencies - Configure topic with env name File Changes - *M* .gitignore https://github.com/openzipkin/zipkin-go/pull/142/files#diff-0 (1) - *M* README.md https://github.com/openzipkin/zipkin-go/pull/142/files#diff-1 (3) - *M* go.mod https://github.com/openzipkin/zipkin-go/pull/142/files#diff-2 (14) - *M* go.sum https://github.com/openzipkin/zipkin-go/pull/142/files#diff-3 (71) - *A* reporter/pubsub/pubsub.go https://github.com/openzipkin/zipkin-go/pull/142/files#diff-4 (109) - *A* reporter/pubsub/pubsub_test.go https://github.com/openzipkin/zipkin-go/pull/142/files#diff-5 (81) Patch Links: - https://github.com/openzipkin/zipkin-go/pull/142.patch - https://github.com/openzipkin/zipkin-go/pull/142.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#142 <#142>?email_source=notifications&email_token=AAAPVVY2D4SHQ5O7FK7PXMTQDE4JXA5CNFSM4IJUCARKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HDSKWZA>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAPVV2NWMZODQK4VFOEFLDQDE4JXANCNFSM4IJUCARA . @adriancole <https://github.com/adriancole> we are evaluating 2 options... most probably we will add a new collector in https://github.com/javierviera/zipkin/tree/master/zipkin-collector but we are also evaluating create a private google cloud function in GCP which pull Google PubSub message and add it to zipkin by API ( https://zipkin.io/zipkin-api/#/default/post_spans) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#142?email_source=notifications&email_token=AAAPVV3MFE7BPCVVWBT37WTQDJUTPA5CNFSM4IJUCARKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3XLTLA#issuecomment-518961580>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV5CHM67DJS74Q5IOK3QDJUTPANCNFSM4IJUCARA> .

reporter/gcppubsub/pubsub.go

javierviera · 2019-08-10T20:45:14Z

Thanks @adriancole Why not creating a new collector in https://github.com/javierviera/zipkin/tree/master/zipkin-collector instead of updating zipkin-gcp? Seems like zipkin-gcp if for having stackdriver as storage.... My idea its just add a new transport.... Am i missing something?

jcchavezs · 2019-08-10T21:02:22Z

@javierviera basically that. You create a goroutine when starting the reporter and listen to a channel signal and a termination signal sent by the *Close* method.

codefromthecrypt · 2019-08-10T23:46:00Z

@javierviera I just raised this issue openzipkin/zipkin-gcp#132

javierviera · 2019-09-09T10:21:26Z

@jcchavezs is there anything pending in the PR?

reporter/gcppubsub/pubsub.go

jcchavezs · 2019-09-09T10:32:39Z

reporter/gcppubsub/pubsub_test.go

+	}
+
+	_, err = client.CreateTopic(ctx, topicID)
+	if err != nil {


I think the best here is that instead of returning a nil we pass the t *testing.T variable into the setup and fatal (t.Fatal) if any of this errs is not nil.
What do you think?

It was like that originally. But the problem is that it would always fail in a CI server or everywhere where GOOGLE_CLOUD_PROJECT is not configured. As the test topic is created in an exiting GCP project... We we test the error but cannot test the success setup...

reporter/gcppubsub/pubsub_test.go

jcchavezs · 2019-09-09T10:36:40Z

Hi @javierviera sorry for the late response. I added a couple of comments, I think we are very close. Great work!

KeisukeYamashita · 2019-11-26T17:35:59Z

@javierviera Any updates? If you don't have time, I can help you:)

jcchavezs · 2019-11-26T18:27:41Z

That would be awesome @keke

javierviera · 2019-11-27T10:30:30Z

Hi folks,

I just improved the unit tests as required. Please check.

fjvierap added 5 commits December 17, 2018 00:41

Add GCP pubsub reporter

058ff83

Merge remote-tracking branch 'upstream/master'

109b9d7

# Conflicts: # go.mod # go.sum

Merge remote-tracking branch 'upstream/master'

092e941

# Conflicts: # go.mod # go.sum

Add Pubsub dependencies

624e689

Configure topic with env name

f9914a5

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/pubsub/pubsub.go Outdated Show resolved Hide resolved

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/pubsub/pubsub.go Outdated Show resolved Hide resolved

fjvierap added 2 commits August 6, 2019 11:40

Update package and rename default topic

c0594a6

Rename default topic env var

692e01b

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub_test.go Show resolved Hide resolved

Improve tests

c67a949

Improve tests

88ecaf9

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

golangcibot reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub_test.go Outdated Show resolved Hide resolved

jcchavezs reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

fjvierap added 2 commits August 6, 2019 13:45

Improve comments

d38ff1e

Check send result non blocking

bb39103

golangcibot reviewed Aug 6, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

Check send result non blocking

537d8e6

basvanbeek reviewed Aug 8, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

basvanbeek reviewed Aug 8, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

basvanbeek reviewed Aug 8, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

jcchavezs reviewed Aug 8, 2019

View reviewed changes

javierviera added 2 commits August 10, 2019 21:11

Only accept collector creation with a valid pubsub client

e6cd074

Goroutine in function

19469f3

golangcibot reviewed Aug 10, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

Fix lint issue

a3fb7f9

codefromthecrypt mentioned this pull request Aug 10, 2019

Implement pubsub openzipkin/zipkin-gcp#132

Open

javierviera added 2 commits August 15, 2019 22:49

Report send status in a single goroutine

a85b23e

Report send status in a single goroutine

0cf6684

jcchavezs reviewed Sep 9, 2019

View reviewed changes

reporter/gcppubsub/pubsub.go Outdated Show resolved Hide resolved

jcchavezs reviewed Sep 9, 2019

View reviewed changes

reporter/gcppubsub/pubsub_test.go Outdated Show resolved Hide resolved

jcchavezs reviewed Sep 9, 2019

View reviewed changes

reporter/gcppubsub/pubsub_test.go Outdated Show resolved Hide resolved

jcchavezs reviewed Sep 9, 2019

View reviewed changes

reporter/gcppubsub/pubsub_test.go Show resolved Hide resolved

Set default topic only when not configured as option

5744523

KeisukeYamashita mentioned this pull request Nov 26, 2019

Add GCP Cloud PubSub reporter #159

Closed

Improve unit tests

53eecf3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Google Cloud Pub Sub reporter #142

Add Google Cloud Pub Sub reporter #142

javierviera commented Aug 6, 2019 •

edited

Loading

jcchavezs Aug 6, 2019 •

edited

Loading

jcchavezs commented Aug 6, 2019

coveralls commented Aug 6, 2019 •

edited

Loading

codefromthecrypt commented Aug 7, 2019 via email

javierviera commented Aug 7, 2019 •

edited

Loading

jcchavezs Aug 8, 2019 •

edited

Loading

basvanbeek Aug 8, 2019

codefromthecrypt commented Aug 9, 2019 via email

javierviera commented Aug 10, 2019 •

edited

Loading

jcchavezs commented Aug 10, 2019 via email •

edited

Loading

codefromthecrypt commented Aug 10, 2019

javierviera commented Sep 9, 2019

jcchavezs Sep 9, 2019 •

edited

Loading

javierviera Sep 9, 2019

jcchavezs commented Sep 9, 2019

KeisukeYamashita commented Nov 26, 2019

jcchavezs commented Nov 26, 2019 via email •

edited

Loading

javierviera commented Nov 27, 2019

Add Google Cloud Pub Sub reporter #142

Are you sure you want to change the base?

Add Google Cloud Pub Sub reporter #142

Conversation

javierviera commented Aug 6, 2019 • edited Loading

jcchavezs Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

jcchavezs commented Aug 6, 2019

coveralls commented Aug 6, 2019 • edited Loading

codefromthecrypt commented Aug 7, 2019 via email

javierviera commented Aug 7, 2019 • edited Loading

jcchavezs Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

basvanbeek Aug 8, 2019

Choose a reason for hiding this comment

codefromthecrypt commented Aug 9, 2019 via email

javierviera commented Aug 10, 2019 • edited Loading

jcchavezs commented Aug 10, 2019 via email • edited Loading

codefromthecrypt commented Aug 10, 2019

javierviera commented Sep 9, 2019

jcchavezs Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

javierviera Sep 9, 2019

Choose a reason for hiding this comment

jcchavezs commented Sep 9, 2019

KeisukeYamashita commented Nov 26, 2019

jcchavezs commented Nov 26, 2019 via email • edited Loading

javierviera commented Nov 27, 2019

javierviera commented Aug 6, 2019 •

edited

Loading

jcchavezs Aug 6, 2019 •

edited

Loading

coveralls commented Aug 6, 2019 •

edited

Loading

javierviera commented Aug 7, 2019 •

edited

Loading

jcchavezs Aug 8, 2019 •

edited

Loading

javierviera commented Aug 10, 2019 •

edited

Loading

jcchavezs commented Aug 10, 2019 via email •

edited

Loading

jcchavezs Sep 9, 2019 •

edited

Loading

jcchavezs commented Nov 26, 2019 via email •

edited

Loading