XRay Remote Service Name is always "Remote"

[codefromthecrypt]

From @cemo on October 18, 2017 21:20

I am in the process of customizing brave and putting into the production system but I came across a problem.

Despite of creating my interceptor with hardcoded labels, it always displays remote in the console. I could not find time to check it but seems there is a bug at there. I might give a try tomorrow to find culprit.

Copied from original issue: openzipkin/brave#524

[cemo]

@adriancole seems this part should be splitted into if/else.

https://github.com/openzipkin/zipkin-aws/blob/master/storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java#L57

      if (span.kind() != null) writer.name("namespace").value("remote");

to

      if (span.kind() != null) {
           if(span.remoteEndpoint() != null && span.remoteEndpoint().serviceName() != null ){
                 writer.name("namespace").value(span.remoteEndpoint().serviceName());
           }
           else {
                writer.name("namespace").value("remote");
           }
      }

Would you accept a PR from me? :)

[codefromthecrypt]

Lemme just check semantics of namespace quickly

…

On 19 Oct 2017 22:23, "Cemalettin Koc" ***@***.***> wrote: @adriancole <https://github.com/adriancole> seems this part should be splitted into if/else. https://github.com/openzipkin/zipkin-aws/blob/master/ storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/ UDPMessageEncoder.java#L57 if (span.kind() != null) writer.name("namespace").value("remote"); to if (span.kind() != null) { if(span.remoteEndpoint() != null && span.remoteEndpoint().serviceName() != null ){ writer.name("namespace").value(span.remoteEndpoint().serviceName()); } else { writer.name("namespace").value("remote"); } } Would you accept a PR from me? :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61xm_3yk6S9V65CKVWgCJfgmOmPhvks5st6HEgaJpZM4P-sWx> .

[codefromthecrypt]

Looks like the name should be set in the condition where we check span.kind If span.kind != null set subsegment set namespace=remote set name = remoteServiceName Else set name = localServiceName http://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html#api-segmentdocuments-subsegments On 19 Oct 2017 22:23, "Cemalettin Koc" <notifications@github.com> wrote: @adriancole <https://github.com/adriancole> seems this part should be splitted into if/else. https://github.com/openzipkin/zipkin-aws/blob/master/ storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/ UDPMessageEncoder.java#L57 if (span.kind() != null) writer.name("namespace").value("remote"); to if (span.kind() != null) { if(span.remoteEndpoint() != null && span.remoteEndpoint().serviceName() != null ){ writer.name("namespace").value(span.remoteEndpoint().serviceName()); } else { writer.name("namespace").value("remote"); } } Would you accept a PR from me? :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61xm_3yk6S9V65CKVWgCJfgmOmPhvks5st6HEgaJpZM4P-sWx> .

[cemo]

Seems you are right. I will give a try as early as possible.

[cemo]

@adriancole sorry for late reply. I have checked and modified as you wrote. Seems "remote" is mandatory. It starts to become visible when I set "http.url". I have checked brave's code but I could not see a place where "http.url" is set. Here is the result when I override HttpServerParser slightly:

One another missing part is user of segment. How should I set it?

user – A string that identifies the user who sent the request.

seems beneficial. I could not see user in Encoder.

[codefromthecrypt]

No worries.. seems like we might need a default http parser for x-ray, to pick the right fields. Ex http.url is not default https://github.com/openzipkin/brave/blob/master/instrumentation/http/README.md#span-data-policy Wanna try to make one for client and server that picks the best things?

[cemo]

I would be happy if I can contribute. Do you mean to add a new HttpServerParser and HttpClientParser xray specific?

[cemo]

@adriancole Should I add a new module as zipkin-aws-http?

edit: Maybe zipkin-aws-instrumentation-http to align with brave-instrumentation-http?

edit2: I have noticed that there is no relation with Brave. So where is the right place to add these additions? The problem is that conceptually Brave has no idea about XRay and zipkin-aws has no idea about Brave since it provides only zipkin core related classes such as Reporter, Collector etc... I could not be sure that where should these classes should reside.

[codefromthecrypt]

I would be happy if I can contribute. Do you mean to add a new HttpServerParser and HttpClientParser xray specific?

yeah, it will probably end up as brave-instrumentation-xray or something (because these types are brave specific)

[cemo]

@adriancole I had some difficulties to grab responsibilities of the project and modules. There is zipkin, zipkin-reporter-java, brave, zipkin-aws. Since I had some difficulties in modules I could not be sure that this new module where it should be belongs to. In brave there is also aws-propagation module as well. It is dependency free and not including anything from zipkin-aws. Now we are evaluating adding a new module in this project as brave-instrumentation-xray and it will has dependencies about brave. This can be easily chicken-egg problem since both project in future can be dependent to each other. I am suggesting to either to add this new module in brave or move aws-propagation to here which sound more logical at first glance to me. Please note that I am very new to this land and my limited knowledge may be not correct.

[codefromthecrypt]

This can be easily chicken-egg problem since both module in future can be

dependent to each other. I am suggesting to either to add this new module in brave or move aws-propagation to here which sound more logical at first glance to me. Please note that I am very new to this land and my limited knowledge may be not correct.

It was a bad suggestion of mine to have the brave code here, as this repo is only about out-of-band architecture (transport (reporter/collector) and storage). The reporter piece is here because zipkin-reporter isn't just brave (other libraries use it). Instrumentation (propagation, tagging policy etc) indeed live where the code does, in brave. So, at the point we're ready to merge some helpers, it would be in the brave repo, you're right. I wouldn't move propagation code here, as it depends on brave classes. Anyway I think you've got the relationships right, and indeed the UDP component being here will feel a bit uncomfortable until things stabilize, but on the other hand it is most like existing code here.

[cemo]

@adriancole I am in the process of supporting all remaining parts of XRay and will add necessary changes in Brave. I also made some progress on Brave as well. I will provide feature parity with documentation as much as possible.

[codefromthecrypt]

thx for the trail blazing!

[cemo]

@adriancole I had some progress in Encoder part. Basically I added

1. Support for SQL
2. Error Handling
3. Using Error Responses to visualize in UI. (You can see in attached screens below)
4. Option to override remote namespace
5. AWS internal components such as DynamoDB, SQS to visualize correctly

There might some parts as well I touched but now I will switch to Brave for refactoring parts you suggested. You can review changes at #59

Some parts are still requiring to be polished especially cause related parts.

I think that it is enough for a start.

[codefromthecrypt]

@cemo champion!

[trajano]

I am thinking this may be related to my SO question.

I haven't verified yet but looking through the code I may just need to alter the default tracing to add the additional details.

@Bean
public HttpTracing httpTracing(Tracing tracing) {

    return HttpTracing.newBuilder(tracing)
        .serverRequestParser(
            (req, context, span) -> {
                HttpRequestParser.DEFAULT.parse(req, context, span);
                HttpTags.URL.tag(req, context, span);
            }
        )
        .serverResponseParser(
            ((response, context, span) -> {
                HttpResponseParser.DEFAULT.parse(response, context, span);
                HttpTags.STATUS_CODE.tag(response, span);
            })
        )
        .clientRequestParser(
            (req, context, span) -> {
                HttpRequestParser.DEFAULT.parse(req, context, span);
                HttpTags.URL.tag(req, context, span); // add the url in addition to defaults
            }
        )
        .clientResponseParser(
            ((response, context, span) -> {
                HttpResponseParser.DEFAULT.parse(response, context, span);
                HttpTags.STATUS_CODE.tag(response, span);
            })
        )
        .build();

}

The mappings in that code block should be reflected in https://github.com/openzipkin/zipkin-aws/blob/master/storage/xray-udp/README.md