search

openzipkin / zipkin-aws

Reporters and collectors for use in Amazon's cloud
Apache License 2.0
69 stars 34 forks source link

Enhancements and fixes to support Istio distributed tracing #170

Closed iamsouravin closed 4 years ago

iamsouravin commented 4 years ago

Update logic for trace id, segment name and origin fields. Add cache for trace id correlation. Update README.md. Add kubernetes artifacts.

codefromthecrypt commented 4 years ago

Hi sourav. thanks for the changes, but there are too many types of change here for one pull request. Can you please separate out the changes into individual ones? For example, whatever the k8s/istio thing should definitely be a different change.

The two minor changes can be discussed individually as one is burdened by a guava dep discussion and the other isn't. I have quite a lot of feedback for the epoch/128-bit one actually, but I don't want to put that in a mixed issue.

Ex when PRs are separate (ideally referencing issues that they fix where possible, ex who asked for istio in the past and how this solves them)... it is easier to inventory features. Also someone can discuss k8s/istio separately from nuance of string parsing. Make sense?

PS. we use files named RATIONALE.md for extended exaplanations.. you can see a fair amount of them in Brave.

iamsouravin commented 4 years ago

Hi sourav. thanks for the changes, but there are too many types of change here for one pull request. Can you please separate out the changes into individual ones? For example, whatever the k8s/istio thing should definitely be a different change.

The two minor changes can be discussed individually as one is burdened by a guava dep discussion and the other isn't. I have quite a lot of feedback for the epoch/128-bit one actually, but I don't want to put that in a mixed issue.

Ex when PRs are separate (ideally referencing issues that they fix where possible, ex who asked for istio in the past and how this solves them)... it is easier to inventory features. Also someone can discuss k8s/istio separately from nuance of string parsing. Make sense?

PS. we use files named RATIONALE.md for extended exaplanations.. you can see a fair amount of them in Brave.

Hi Adrian, thanks for the tips. I'm creating other more focused PRs as suggested and will update this one.