weixin_40003478
weixin_40003478
2020-12-01 16:59

exporter/zipkin: option to set RemoteEndpoint

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

该提问来源于开源项目:census-instrumentation/opencensus-go

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

6条回答

  • weixin_40003478 weixin_40003478 5月前

    Thanks for the first pass of review !

    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.

    点赞 评论 复制链接分享
  • weixin_39928106 weixin_39928106 5月前

    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?

    点赞 评论 复制链接分享
  • weixin_40003478 weixin_40003478 5月前

    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 https://github.com/census-instrumentation/opencensus-go/blob/96e75b88df843315da521168a0e3b11792088728/exporter/zipkin/zipkin.go#L53-L55

    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.

    点赞 评论 复制链接分享
  • weixin_39668408 weixin_39668408 5月前

    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: https://github.com/census-instrumentation/opencensus-specs/issues/135

    点赞 评论 复制链接分享
  • weixin_39836943 weixin_39836943 5月前

    -em should this be closed until there is consensus on how to fix this by resolving spec issue first (https://github.com/census-instrumentation/opencensus-specs/issues/135).

    点赞 评论 复制链接分享
  • weixin_39545269 weixin_39545269 5月前

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

    点赞 评论 复制链接分享

相关推荐