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

Tracing: NATS support #1006

Open
sergey-suslov opened this issue Aug 19, 2020 · 7 comments
Open

Tracing: NATS support #1006

sergey-suslov opened this issue Aug 19, 2020 · 7 comments

Comments

@sergey-suslov
Copy link

sergey-suslov commented Aug 19, 2020

Hi. I would like to add NATS support in the go-kit tracing.

How to pass a span context?

Unlike gRPC or HTTP packages, NATS doesn't have any headers or metadata (as I know) to pass content besides request body. Due to this, I see the only solution is to pass the span context within the message data.

There are two options, that come to mind:

  • pass the context and the data in separate fields (tracing has to be plugged in on the other end)
type natsMessageWithContext struct {
	Sc   model.SpanContext `json:"sc"`
	Data interface{}       `json:"data"`
}
  • pass the context among other data fields (doesn't look good, from my point of view)

For the rest, I suggest to do by analogy with the gRPC tracing and HTTP tracing solutions.

Is there something, that may be a problem? Is this solution acceptable?

Samples:
Publisher - https://github.com/sergey-suslov/apartments-booking/blob/feature/nats-tracing-as-options/services/booking/pkg/nats-tracing/nats.go
Subscriber - https://github.com/sergey-suslov/apartments-booking/blob/feature/nats-tracing-as-options/services/apartments/pkg/nats-tracing/nats.go

@peterbourgon
Copy link
Member

I think it's a good idea, but I don't think the code necessarily needs to live in the Go kit repo. Can you prototype it somewhere and point me to it for review? If it's making sense, I'm happy to link to it from Go kit in the relevant places.

@sergey-suslov
Copy link
Author

I think it's a good idea, but I don't think the code necessarily needs to live in the Go kit repo. Can you prototype it somewhere and point me to it for review? If it's making sense, I'm happy to link to it from Go kit in the relevant places.

Ok. I'll prototype it and notify you when it's ready.

@basvanbeek
Copy link
Member

I quickly glanced at some items... For pub/sub you can use the producer and consumer kinds.

For more info on pubsub tracing with Zipkin, check out: https://zipkin.io/pages/instrumenting (specifically the Message Tracing section).

@sergey-suslov
Copy link
Author

I quickly glanced at some items... For pub/sub you can use the producer and consumer kinds.

For more info on pubsub tracing with Zipkin, check out: https://zipkin.io/pages/instrumenting (specifically the Message Tracing section).

Thanks.

@wallyqs
Copy link
Contributor

wallyqs commented Aug 21, 2020

Hi @sergey-suslov, just as a heads up that after almost 10 years the NATS protocol has been updated to support headers (here: nats-io/nats.go#560), although this has not been released yet in the server nor have we tagged the client that includes it. Also your approach might be the most compatible with previous versions of the client without having to rely on the latest version of the NATS client, but just thought I'd share that native support for headers is coming in one of the next releases as well.

@sergey-suslov
Copy link
Author

Hi @sergey-suslov, just as a heads up that after almost 10 years the NATS protocol has been updated to support headers (here: nats-io/nats.go#560), although this has not been released yet in the server nor have we tagged the client that includes it. Also your approach might be the most compatible with previous versions of the client without having to rely on the latest version of the NATS client, but just thought I'd share that native support for headers is coming in one of the next releases as well.

Big thanks. Anyway, I am gonna implement the "in-message context transporting" approach. Then, I believe, using headers as a context holder should be added as an option.

@sergey-suslov
Copy link
Author

@peterbourgon I have made a prototype https://github.com/sergey-suslov/go-kit-nats-zipkin-tracing
What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants