Skip to content
This repository has been archived by the owner on Dec 5, 2023. It is now read-only.

Add local IP address to Zipkin tracing #53

Merged

Conversation

embs
Copy link
Contributor

@embs embs commented Sep 21, 2017

Also reuse client span to be consistent with how interactions between
Java services are traced.
UPDATE: removed as Zipkin is moving away from client/server shared spans: openzipkin/zipkin#939

Related to microservices-demo/orders#48

An example of how these changes affect traces:

Before modification

selection_005

Complete trace: original.zip

After modification

selection_006

Complete trace: after-modification.zip

The highlighted frames in second image refers to:

  1. Go service overriding operation name in span. ❌ Do not always override operation name when tracing endpoints go-kit/kit#615
  2. Go service adding SR and SS annotations to the span. ✅
  3. Go service adding binnary annotations which are also added by Java (caller) service ❌ Activating ClientServerSameSpan resulted in duplicated binnary annotations openzipkin-contrib/zipkin-go-opentracing#81

1 and 3 are not huge problems but I'm working on them in order to achieve consistence with how Java <-> Java services interactions are traced.

@embs embs changed the title Add local IP address to Zipkin tracing WIP: Add local IP address to Zipkin tracing Sep 21, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 23.465% when pulling 20be7a4 on gfads:enhancement/zipkin-tracing into b5ec77e on microservices-demo:master.

@embs
Copy link
Contributor Author

embs commented Sep 22, 2017

I've removed config for sharing span as Zipkin is moving away from client/server shared spans: openzipkin/zipkin#939

So final format looks like this:

Client span

selection_004

Server span

selection_005

Complete trace: trace.zip

@coveralls
Copy link

Coverage Status

Coverage remained the same at 23.465% when pulling 52923f9 on gfads:enhancement/zipkin-tracing into b5ec77e on microservices-demo:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 23.465% when pulling 10b11fe on gfads:enhancement/zipkin-tracing into 013b519 on microservices-demo:master.

Copy link
Contributor

@vlal-zz vlal-zz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vlal-zz vlal-zz merged commit a7ea691 into microservices-demo:master Nov 1, 2017
@embs embs deleted the enhancement/zipkin-tracing branch November 7, 2017 11:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants