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

cemo · 2017-11-04T18:38:07Z

No description provided.

cemo · 2017-11-04T21:14:52Z

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

…ynamoDB or SQS

codefromthecrypt

Looks like good work. We are accumulating some tech-debt. Can you try to add a unit test? for example, in the test instantiate a zipkin2.Span with the input data and test the output json on string equality (ex assertThat(json).isEqualTo("{...}").

codefromthecrypt · 2017-11-08T08:12:25Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java

@@ -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());


do we need to write empty name? or is leaving out name tolerable? If possible, I'd prefer to not write placeholder values

I will test it and let you know.

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:

name it "unknown"

ignore span

Which one seems better for you @adriancole ?

codefromthecrypt · 2017-11-08T08:19:10Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java

    }
-    writer.name("name").value(span.localServiceName());
+    // override with the user remote tag


I don't think we should do it like this. we are setting the value of the xray field "namespace" to the value of a zipkin key "remote".

per amazon "Any calls to a remote service/broker should be namespace as ‘remote’. See ‘Optional Subsegment Fields’ under http://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html#api-segmentdocuments-subsegments."

So, we should automatically default "namespace" to "remote" when it is a downstream PRODUCER or CLIENT span. If you look in the docs, the other supported choice is "aws". We don't support that in brave, yet, but we could.. in that case, I'd recommend using the zipkin key "xray.namespace" as a way to conditionally override.

make sense?

xray.namespace sound good but all other variables are using something similar to JSON path dot notation.

I will change it as you suggest.

If I instrument a method (It might not be available at the time of this writing), what kind of span it will be? Since it is not connecting something else, I have some doubts on this.

Im referring about a tag key ex span.tag("xray.namespace", "aws"). Reporter only sends complete spans.. how would we have a problem in routine use? For example there are only two values remote (we know) and aws (solved as soon as we write aws sdk instrumentation or integrate with it). Users will not do this manually in other words

codefromthecrypt · 2017-11-08T08:19:54Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/XRayFormatter.java

+import java.io.IOException;
+import okio.Buffer;
+
+public final class XRayFormatter {


Add docs to how you came up with the format.. also make the type not public unless required to be. If required to be, mention why.

This is a utility class, basically brave.SpanCustomizer#tag is using <String, String> we can not pass an exception to it. So I had to prepare JSON to be passed to subsegments. Here is the place which is used https://github.com/openzipkin/zipkin-aws/pull/59/files#diff-5669c8fcacad45292f4bc1d457640d24R243. This is the dirtiest place since it is not using JsonWriter. Could you review this part again please? If you are agree with this solution I will come up some documents regarding your notes before you stated.

Here is the relevant docs on XRay as well: The structure is deeply nested and there is not an easy solution with the current state.
http://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html#api-segmentdocuments-errors
I had also decompiled XRay source codes and found that place overly complicated.

That formatter class is not something very nice but it gives some information about exception and also let the user to use another logic.

cemo · 2017-11-08T18:59:04Z

I will add more test regarding Encoder.

codefromthecrypt · 2017-11-09T00:12:00Z

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" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/ UDPMessageEncoder.java <#59 (comment)>: > @@ -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 <#59 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61ww8upy_aq5HWXnfwWzZf5lWIYOLks5s0ayJgaJpZM4QSHIk> .

codefromthecrypt · 2017-11-09T00:13:52Z

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" ***@***.***> 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" ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPM > essageEncoder.java > <#59 (comment)>: > > > @@ -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 > <#59 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAD61ww8upy_aq5HWXnfwWzZf5lWIYOLks5s0ayJgaJpZM4QSHIk> > . >

codefromthecrypt · 2017-11-09T00:18:17Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/XRayFormatter.java

+   *
+   * Formating an exception to be consumed by XRay by Spans annotations
+   *
+   * @param exceptionId an unique exception id @see brave.internal.Platform#randomLong()


Is this an ID? Sometimes I have seen hashes for this .. ex hash of the stack trace. Either way i would hide this type, using internally in aws specific http and sql parsers. Hard to change public types

Doc: id – A 64-bit identifier for the exception, unique among segments in the same trace, in 16 hexadecimal digits.

Probably can reuse span ID as zipkin instrumentation only record one error tag per span

codefromthecrypt · 2017-11-09T00:20:08Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/XRayFormatter.java

+   * Formating an exception to be consumed by XRay by Spans annotations
+   *
+   * @param exceptionId an unique exception id @see brave.internal.Platform#randomLong()
+   * @param isRemote Any calls to a remote service/broker should be true


Presume applies to client and server side?

Doc: remote – boolean indicating that the exception was caused by an error returned by a downstream service.

codefromthecrypt · 2017-11-09T00:21:10Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/XRayFormatter.java

+   *
+   * @param exceptionId an unique exception id @see brave.internal.Platform#randomLong()
+   * @param isRemote Any calls to a remote service/broker should be true
+   * @param maxStackTraceElement XRay has a limit of 60KB per segment. Choose a wise number


This would be global configurable as ex transports have limits that can be much smaller

How can I configure this number since the method is static? Should I declare another static variable can be changeable? Or should I make it static class and force users to create an instance of it?

codefromthecrypt · 2017-11-09T00:23:10Z

storage-xray-udp/src/test/java/zipkin2/storage/xray_udp/UDPMessageEncoderTest.java

+        .shared(false)
+        .build();
+
+    byte[] bytes = UDPMessageEncoder.doEncode(span);


Nit i would extract a method that only encodes span json as that lets us in future do json asserts

I got your point. I made it JSON assertions friendly.

codefromthecrypt

I would prefer to defer exception mapping as it is complex to map and includes state assumptions.. it is better imho to get the easier stuff done and landed before harder stuff. What do you think? Punt the stack trace parsing to a separate issue (which we can follow up on)

codefromthecrypt · 2017-11-09T11:20:43Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java

@@ -121,11 +201,56 @@
      writer.endObject();
    }

+    Integer errorStatus = httpResponseStatus;


We have standard http tag for this. http.status_code I believe. For input tags, reuse zipkin ones and namespace (ex xxxx.foo) where exist

If there is an http error and available http.status_code, I am using it. But there might be some other errors as well. For example a DB exception or something else. For that part our instrumentation codes must provide error_status.

For input tags, reuse zipkin ones and namespace (ex xxxx.foo) where exist

I could not get this, can you expand please

Mainly it is getting cognitively difficult to follow things. Let's try not to move ahead of the ball, as there's an advantage in x-ray which is that it is model based. Instead of creating error_status which isn't defined anywhere in zipkin, for possible correlated use between http and sql, perhaps define them separately, in namespaces such as http.status_code (and whatever it is for sql, ex sql.status_code). Eventhough I have more time than others I'm having trouble following this, so lets try and keep it organized as possible and using zipkin conventions whereever, and without support for hypothetical things (ex only code things you have a test case and actual brave instrumentation to support)

There might be other services can cause status_code. How can I add status_code for a redis call or a method instrumentation. If you believe that it is confusing. Let's
change error_status to xray.error_status since it is a top level property of Subsegments.

codefromthecrypt · 2017-11-09T11:34:02Z

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 · 2017-11-09T12:41:27Z

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 · 2017-11-09T12:45:00Z

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

codefromthecrypt · 2017-11-09T12:55:10Z

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 · 2017-11-09T12:59:04Z

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 · 2017-11-10T11:03:12Z

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

codefromthecrypt

One test nit. Please add http server and client tests based on data brave produces.

Also remove the error section as it isnt tested or referenced yet. Reintroduce it later.

Good stuff will merge after above

codefromthecrypt · 2017-11-10T11:41:23Z

storage-xray-udp/src/test/java/zipkin2/storage/xray_udp/UDPMessageEncoderTest.java

+        .newBuilder()
+        .kind(Span.Kind.CLIENT)
+        .name("test-cemo")
+        .remoteEndpoint(Endpoint.newBuilder().build())


Empty endpoint is not normal. Better left unset

codefromthecrypt · 2017-11-10T11:42:22Z

storage-xray-udp/src/test/java/zipkin2/storage/xray_udp/UDPMessageEncoderTest.java

+  }
+
+  @Test
+  public void doEncodeUnkown() throws Exception {


Maybe encode_nameAsUnknownWhenRemoteServiceNameNull

codefromthecrypt · 2017-11-20T05:36:46Z

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

cemo · 2017-11-20T05:47:25Z

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

codefromthecrypt · 2017-11-20T05:50:45Z

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 · 2017-12-04T21:32:54Z

I will do some polishing post-merge.. promise

tabdulradi · 2017-12-05T14:06:05Z

storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java

+      // using "unknown" subsegment name will help to detect missing names
+      writer.name("name").value(span.remoteServiceName() == null ? "unknown" : span.remoteServiceName());
+    }else{
+      writer.name("name").value(span.localServiceName());


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.

codefromthecrypt · 2017-12-05T16:08:17Z

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

Fixed xray subsegment naming

1bac4a3

cemo force-pushed the b-xray-subsegment-naming branch 2 times, most recently from 144e823 to 493f549 Compare November 6, 2017 15:09

cemo added 4 commits November 7, 2017 17:38

Adds support for xray sql subsegment

e752448

Adds optional namespace resolution for aws related services such as D…

4ce69c1

…ynamoDB or SQS

Adds support for xray aws subsegment

738d469

Adds support for error status

17585e9

cemo force-pushed the b-xray-subsegment-naming branch 3 times, most recently from cd6c570 to 97c3223 Compare November 7, 2017 18:11

Adds support for cause section in subsegments

58f082e

cemo force-pushed the b-xray-subsegment-naming branch 2 times, most recently from 27dfb33 to c3090d9 Compare November 8, 2017 07:20

Fixes edge cases in null conditions

eb81fda

cemo force-pushed the b-xray-subsegment-naming branch from c3090d9 to eb81fda Compare November 8, 2017 07:24

Adds license to XRayFormatter class

a91164d

cemo mentioned this pull request Nov 8, 2017

XRay Remote Service Name is always "Remote" #58

Open

codefromthecrypt reviewed Nov 8, 2017

View reviewed changes

Add some tests and refactored XRay related codes

ef1fed5

cemo force-pushed the b-xray-subsegment-naming branch from 40a6576 to ef1fed5 Compare November 8, 2017 19:00

codefromthecrypt reviewed Nov 9, 2017

View reviewed changes

Add extra tests and addressed issues on reviews

5d5f678

codefromthecrypt reviewed Nov 9, 2017

View reviewed changes

Deletes XRayFormatter to address in a separeted issue

fbeb31f

Removed error_status support to be addressed in a separated issue

0dc3c7d

cemo changed the title ~~Fixed xray subsegment naming~~ Support XRay features in Encoder Nov 10, 2017

cemo changed the title ~~Support XRay features in Encoder~~ Support XRay features (Sql, Cause, aws) in Encoder Nov 10, 2017

codefromthecrypt reviewed Nov 10, 2017

View reviewed changes

codefromthecrypt merged commit d282aca into openzipkin:master Dec 4, 2017

codefromthecrypt mentioned this pull request Dec 4, 2017

Ensures X-Ray json has parent_id if type=segment #63

Closed

tabdulradi reviewed Dec 5, 2017

View reviewed changes

robyf mentioned this pull request May 25, 2018

Sending "unknown" as segment name to X-Ray results in incorrect data displayed in the X-Ray UI #87

Closed

robyf mentioned this pull request Jun 25, 2018

Use local service name as X-Ray subsegment name #88

Closed

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

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

Conversation

cemo commented Nov 4, 2017

cemo commented Nov 4, 2017

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemo commented Nov 8, 2017

codefromthecrypt commented Nov 9, 2017 via email

codefromthecrypt commented Nov 9, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemo Nov 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cemo Nov 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Nov 9, 2017

codefromthecrypt commented Nov 9, 2017

cemo commented Nov 9, 2017

codefromthecrypt commented Nov 9, 2017 via email

cemo commented Nov 9, 2017

cemo commented Nov 10, 2017 • edited

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Nov 20, 2017

cemo commented Nov 20, 2017

codefromthecrypt commented Nov 20, 2017 via email

codefromthecrypt commented Dec 4, 2017

Choose a reason for hiding this comment

codefromthecrypt commented Dec 5, 2017 via email

cemo Nov 9, 2017 •

edited

cemo Nov 9, 2017 •

edited

cemo commented Nov 10, 2017 •

edited