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

Decouple from zipkin2.Endpoint #745

Closed
codefromthecrypt opened this issue Aug 2, 2018 · 2 comments
Closed

Decouple from zipkin2.Endpoint #745

codefromthecrypt opened this issue Aug 2, 2018 · 2 comments

Comments

@codefromthecrypt
Copy link
Member

One thing that hurt us updating from zipkin v1 library to zipkin v2 was the Endpoint type. It caused a fair amount of library dancing and a difficult to remove dependency. For example, assigning a remoteEndpoint caused an overload on a close, but still 3rd party type. Moreover, we know people want to change only specific pieces of the endpoint. For example, just the port for services. Sometimes, like in camel or sidecars, folks want to change the local service name. It can be tricky to do this without accidentally messing up the IP information. Finally, whenever this occurs, there's some overhead due to re-allocating Endpoint objects.

The idea here is to decouple the tracing apis from zipkin2.Endpoint types, instead using a brave cover type, which in the case of NoOp does nothing. Underneath a "MutableEndpoint" type would be used in recording which is only conditionally present when overriding defaults. We'd deprecate the existing apis which ask for zipkin2.Endpoint for removal in Brave 6.

@codefromthecrypt
Copy link
Member Author

Here is a summary of rationale from chat:

Summary:

the context is MutableSpan, the new customizable thing should it be decoupled from the types in io.zipkin.zipkin2. not wrapped really, replicated, then only when reported to zipkin (ex not to micrometer or similar) only then we convert so you could imagine something like...
mutableSpan.localServiceName("foo") which doesn't re-create or in any way reference zipkin2.Endpoint.serviceName

On validation:

otoh we have a choice to make :P Consider mutableSpan.ip("1.3.armadillo.4") Right now, due to some nice code in zipkin2.Endpoint, we can cheaply validate on this call and return false. This allows you to do conditional stuff (ex if (!endpoint.ip(forwardedFor)) endpoint.ip(remoteIp))
so the cost is either not having validation, or making validation utilities, or embed the same validation code into MutableEndpoint.
it is 375 lines of code.. the ip validation stuff I don't think I've had to change it recently.. so anyway this is the source of my tension.. whether to copy/paste this stuff (and the tests) from zipkin2 in order to decouple the libraries for the MutableSpan type.

Less dependency exposure to "non-zipkin" integrations:

If we do this now then, in brave 5.2 we have a way to not have sneaky deps on zipkin2 library, and those who make metrics handlers.. they won't break if we make a zipkin3 (bound to eventually occur). Plus, we are increasingly sending data to places not zipkin as well xray, stackdriver, datadog, etc. .. metrics being even the more odd recipient

This is all related to firehose mode: #557

It is also related to Camel which currently manages multiple instances just to override the local service name they are proxying for. https://github.com/apache/camel/blob/master/components/camel-zipkin/src/main/java/org/apache/camel/zipkin/ZipkinTracer.java#L440

Also this has been reported numerous times sleuth Ex:
spring-cloud/spring-cloud-sleuth#1039
spring-cloud/spring-cloud-sleuth#1041

codefromthecrypt pushed a commit that referenced this issue Aug 4, 2018
This adds methods that decouple Brave apis from zipkin2.Endpoint:
* Tracing.Builder.localIp, localPort for global config
* Span.remoteServiceName, parseRemoteIpAndPort for remote endpoint

Interally, a MutableEndpoint type is used, though this might be
flattened depending on future benchmarks. It is not exposed as a
public api.

Fixes #745
@codefromthecrypt
Copy link
Member Author

#747 should fix this

codefromthecrypt pushed a commit that referenced this issue Aug 6, 2018
This adds methods that decouple Brave apis from zipkin2.Endpoint:
* Tracing.Builder.localIp, localPort for global config
* Span.remoteServiceName, parseRemoteIpAndPort for remote endpoint

Interally, a MutableEndpoint type is used, though this might be
flattened depending on future benchmarks. It is not exposed as a
public api.

Fixes #745
codefromthecrypt pushed a commit that referenced this issue Aug 6, 2018
This adds methods that decouple Brave apis from zipkin2.Endpoint:
* Tracing.Builder.localIp, localPort for global config
* Span.remoteServiceName, remoteIpAndPort for remote endpoint

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

No branches or pull requests

1 participant