Support XRay features (Sql, Cause, aws) in Encoder

[cemo]

Please don't merge yet. I am still testing some cases since span.remoteServiceName() might be null in some cases.

[cemo]

I will add more test regarding Encoder.

[codefromthecrypt]

I would try to get close to user's desire as possible. Usually span name isnt null, but it could be sent as null when updating a span post factum. Maybe set to empty string and add a comment we are doing this to avoid the data being dropped.

On 8 Nov 2017 9:28 pm, "Cemalettin Koc" notifications@github.com wrote:

@cemo commented on this pull request.

In storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/ UDPMessageEncoder.java https://github.com/openzipkin/zipkin-aws/pull/59#discussion_r149667064:

@@ -55,8 +57,13 @@ || span.kind() != Span.Kind.SERVER && span.kind() != Span.Kind.CONSUMER) { writer.name("type").value("subsegment"); if (span.kind() != null) writer.name("namespace").value("remote");

• writer.name("name").value(span.remoteServiceName() == null ? "" : span.remoteServiceName());

when it is null or empty string, it ignores span so it does not appear at all. There is two possibility when it is null or empty:

1. name it "unknown"
2. ignore span

Which one seems better for you @adriancole https://github.com/adriancole ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-aws/pull/59#discussion_r149667064, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD61ww8upy_aq5HWXnfwWzZf5lWIYOLks5s0ayJgaJpZM4QSHIk .

[codefromthecrypt]

Sorry you said empty is not permitted.. yeah unknown is best but make sure there is a comment (and a unit test saying why we do this as we might potentially overwrite a good name with a bad one, unless the way logic is structured it is unlikely..)

On 9 Nov 2017 8:11 am, "Adrian Cole" adrian.f.cole@gmail.com wrote:

I would try to get close to user's desire as possible. Usually span name isnt null, but it could be sent as null when updating a span post factum. Maybe set to empty string and add a comment we are doing this to avoid the data being dropped.

On 8 Nov 2017 9:28 pm, "Cemalettin Koc" notifications@github.com wrote:

@cemo commented on this pull request.

In storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPM essageEncoder.java https://github.com/openzipkin/zipkin-aws/pull/59#discussion_r149667064:

@@ -55,8 +57,13 @@ || span.kind() != Span.Kind.SERVER && span.kind() != Span.Kind.CONSUMER) { writer.name("type").value("subsegment"); if (span.kind() != null) writer.name("namespace").value("remote");

• writer.name("name").value(span.remoteServiceName() == null ? "" : span.remoteServiceName());

when it is null or empty string, it ignores span so it does not appear at all. There is two possibility when it is null or empty:

1. name it "unknown"
2. ignore span

Which one seems better for you @adriancole https://github.com/adriancole ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-aws/pull/59#discussion_r149667064, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD61ww8upy_aq5HWXnfwWzZf5lWIYOLks5s0ayJgaJpZM4QSHIk .

[codefromthecrypt]

By the way, it is indeed easier to review seeing things applied. I think we were discussing making a brave-instrumentation-xray module which would produce the tags consumed here. Seeing together makes it easier to tell style impact. How do you feel about creating that as a maven module? Would you need a hand? Cc @devinsba

[codefromthecrypt]

in zipkin, a generic error is the tag "error" and the UI pays attention to it. for redis, it would likely be redis.error_status if there is something more specific. Lets' please stick to things concrete, it will be more smooth for both of us to only focus on things we can test

[cemo]

OK, I removed it as well. It is better to address it in a separated issue.

[codefromthecrypt]

PS I'd like to help more, just trying to finish cassandra which is very overdue. Notably I'd like to help get the integrated stuff in (ex such that we can connect brave to this work, and test end-to-end) really appreciate you blazing trails and patience with feedback.

[cemo]

No problem @adriancole. It was my pleasure. You are leading this community, the things we are doing nothing when it is compared with your work. I am just new to this land and had some difficulties to grab context. :)

[cemo]

@adriancole is there anything else I can provide for this pr?

[codefromthecrypt]

hi, @cemo sorry if I've asked a lot. Let me know if you need a hand wrapping up last things I mentioned.

[cemo]

No problem but I could not find time to complete. I would be glad if you polish and deliver it.

[codefromthecrypt]

No problem but I could not find time to complete. I would be glad if you polish and deliver it.

sure thing. that's why I asked :) will polish up.

Thanks for all the help

[codefromthecrypt]

I will do some polishing post-merge.. promise

[codefromthecrypt]

hey, I am now getting all my traces labelled as "unkown", previously they used to obey the localServiceName(). How is remoteServiceName supposed to be set? I usually set the localServiceName through Tracing.newBuilder().localServiceName, but there is no similar method for remote service name.

you have to scope your http tracing component when you are a client to assign a remote service name. It is "clientOf"

https://github.com/openzipkin/brave/tree/master/instrumentation/http#span-data-policy

Not sure if it is fine to just leave out the name when we don't know the remote side or not.. we can ask. If you look in the other PR, the test is more obvious https://github.com/openzipkin/zipkin-aws/pull/65/files#diff-cc008c2d18f80edd4dff94bf438c1b4aR72