exporter/zipkin: option to set RemoteEndpoint

[odeke-em]

Provide an option WithRemoteEndpoint that allows
setting the remote endpoint of a Zipkin exporter.
This change is necessary because the constructor
NewExporter only takes in a local endpoint, implying
that remoteEndpoint is optional but when necessary,
we need to send this information along since it is
used by Zipkin to construct a service graph.
I found this issue while working on the OpenCensus service/agent.

Fixes #959

[semistrict]

I'm not sure functional options are the right approach here. Consider just adding a field instead - it's much simpler.

Secondly, I think it would be great if we modified all the examples to set RemoteEndpoint.

[semistrict]

I think this is a lot of ceremony to accomplish this. I know @rakyll also dislikes the functional options pattern. What about just adding an exported field to the exporter and then users can do:

e := zipkin.NewExporter(...)
e.RemoteEndpoint = "..."

[odeke-em]

Unfortunately that makes for an awkward API -- why is localEndpoint an unexported field but RemoteEndpoint is exported? RemoteEndpoint being exported makes it seem then like the important field yet localEndpoint has been the seemingly most mandatory one.

Considering a case by case basis this is the need for an option because:
a) the exporter has been started and localEndpoint which is the more important field isn't exposed
b) for the 1+ year that this exporter has existed, no one has needed remoteEndpoint and am the first, hence the "optionality"

[semistrict]

We should document what the remote endpoint is. It is not obvious.

[odeke-em]

Thanks for the first pass of review @Ramonza!

I'm not sure functional options are the right approach here. Consider just adding a field instead - it's much simpler.

It makes for an awkward API if localEndpoint is added in the constructor and then RemoteEndpoint is a field. Perhaps either we'd have to create a new constructor(am not sure that's advisable) or break the signature of the exporter that has been used for the past 1 year. As I had previously mentioned, am using functional options because RemoteEndpoint has been treated as "optional" for the past 1+ years and I am on the edge of those folks that need it, am not sure that'd warrant a field. The Stackdriver exporter constructor uses a struct with fields so that use case matches properly but this one doesn't.

[semistrict]

I was confusing remote endpoint with local endpoint in my previous comments.

The only documentation I could find describing what exactly the remote endpoint should be is: https://static.javadoc.io/io.zipkin.brave/brave/4.1.0/brave/Span.html#remoteEndpoint-zipkin.Endpoint-

For a client span, this would be the server's address.

This seems like it would be a per-span thing, rather than something that could be set globally on the exporter. For zipkin-go, it is also a per-span field. So I'm a little confused as to why we are we adding it to the exporter - am I missing something here?

[odeke-em]

his seems like it would be a per-span thing, rather than something that could be set globally on the exporter. For zipkin-go, it is also a per-span field. So I'm a little confused as to why we are we adding it to the exporter - am I missing something here?

Right, it is a per span field and so is LocalEndpoint. according to:

Resource	URL
Zipkin-Go	Span definition
Zipkin's data model	https://zipkin.io/zipkin-api/#/default/post_spans

With the OpenCensus service, I am not using one exporter, I have to create a unique exporter per localEndpoint-remoteEndpoint combination in order to use this exporter. Even as is I still need to make many exporters since spans intercepted from Zipkin producing services exist off different localEndpoints.

I think if we were to redesign this exporter:
a) Allow LocalEndpoint and RemoteEndpoint to be attributes in a span e.g. "zipkin.localEndpoint.", "zipkin.remoteEndpoint."
b) if unset we could retrieve the LocalEndpoint from the localIP and host name

Evidently even the exporter's ExportSpan method passes on localEndpoint

opencensus-go/exporter/zipkin/zipkin.go

Lines 53 to 55 in 96e75b8

	func (e *Exporter) ExportSpan(s *trace.SpanData) {
	e.reporter.Send(zipkinSpan(s, e.localEndpoint))
	}

I was contemplating just implementing a fresh exporter to accomodate the per-span localEndpoint+remoteEndpoint needs but I'd prefer we consolidate that work in one place. With my newly proposed design, I'd be able to use just one exporter and it could multiplex on each localEndpoint-remoteEndpoint combination.

[basvanbeek]

The Zipkin datamodel indeed describes both local endpoint and remote endpoint as per span setable items. Since most instrumented services are single homed and not proxying/annotating on behalf of others we typically allow a default local endpoint to be set on the tracer level (ex. Zipkin-go) and can be overridden by spans if needed.

This works great for local endpoints as it's basically self describing their own node in the graph but this won't work for remote endpoints as they typically aren't confined to a single remote service.

For reference I'm adding an issue I raised some time ago on OpenCensus specs to have a better solution for this in OC: census-instrumentation/opencensus-specs#135

[rghetia]

@odeke-em should this be closed until there is consensus on how to fix this by resolving spec issue first (census-instrumentation/opencensus-specs#135).

[songy23]

Closing because zipkin exporter has been moved to https://github.com/census-ecosystem/opencensus-go-exporter-zipkin.