Instrumentation of aws-sdk

[devinsba]

Adds a RequestHandler2 that traces AWS API requests using brave

Closes #53

[devinsba]

I had a commit earlier working around a hiccup in the strategy, this begs the question, do we even want to use AWS's loader functionality at all, or should we make users use the explicit .withRequestHandler() when they create their clients.

I'd prefer the magical loader myself, but I've now written code to guard against if both happen. Thoughts @adriancole @llinder @cemo @pims

[codefromthecrypt]

I wonder if we should. I suspect in spring boot we would prefer to post process. maybe we end up later with another jar like aws-sdk-loader that depends on all things we write including wrappers. meanwhile where frameworks already exist seems explicit allows them to configure more neatly.

[devinsba]

So, in the AWS XRay SDK, they do duplicate instrumentation checking, so its not that crazy to have to do it in ours too

[devinsba]

Quote from XRay documentation (https://docs.aws.amazon.com/xray/latest/devguide/xray-sdk-java-awssdkclients.html):

The X-Ray SDK for Java automatically instruments all AWS SDK clients when you include the aws-sdk and aws-sdk-instrumentor submodules in your build. If you don't include the Instrumentor submodule, you can choose to instrument some clients while excluding others.

I think this is the way forward if someone wants the autoinstrumentation

[codefromthecrypt]

probably 2 different things. path should be accurate. possible that clones are supported with different base url?

for span name.. wwxd? (what would xray do)

On Mon, 23 Apr 2018, 22:17 Brian Devins, notifications@github.com wrote:

@devinsba commented on this pull request.

In brave-instrumentation/aws-java-sdk-core/src/main/java/brave/instrumentation/aws/HttpAdapter.java https://github.com/openzipkin/zipkin-aws/pull/81#discussion_r183411244:

+ +public class HttpAdapter extends HttpClientAdapter<Request<?>, Response<?>> {

• @Override public boolean parseServerAddress(Request<?> request, Endpoint.Builder builder) {
• // Does this belong here?
• builder.serviceName(request.getServiceName());
• InetAddress inetAddress = null;
• try {
• inetAddress = InetAddress.getByName(request.getEndpoint().getHost());
• } catch (UnknownHostException e) {
• return false;
• }
• return builder.parseIp(inetAddress);
• }
• @Override public String method(Request<?> request) {

The path is always empty or / since all their work is done by the RPC nature of the payload. Wondering if method should be the RPC method, and path should be the service name?

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

[codefromthecrypt]

great

On Tue, 24 Apr 2018, 00:13 Brian Devins, notifications@github.com wrote:

Quote from XRay documentation ( https://docs.aws.amazon.com/xray/latest/devguide/xray-sdk-java-awssdkclients.html ):

The X-Ray SDK for Java automatically instruments all AWS SDK clients when you include the aws-sdk and aws-sdk-instrumentor submodules in your build. If you don't include the Instrumentor submodule, you can choose to instrument some clients while excluding others.

I think this is the way forward if someone wants the autoinstrumentation

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

[codefromthecrypt]

yep was thinking similar.. especially if http under ends up instrumented

On Tue, 24 Apr 2018, 01:33 Brian Devins, notifications@github.com wrote:

@devinsba commented on this pull request.

In brave-instrumentation/aws-java-sdk-core/src/main/java/brave/instrumentation/aws/TracingRequestHandler.java https://github.com/openzipkin/zipkin-aws/pull/81#discussion_r183477442:

• }
• };
• final HttpClientHandler<Request<?>, Response<?>> handler;
• final TraceContext.Injector<Request<?>> injector;
• TracingRequestHandler(HttpTracing httpTracing) {
• handler = HttpClientHandler.create(httpTracing, ADAPTER);
• injector = httpTracing.tracing().propagation().injector(SETTER);
• }
• @Override public final void beforeRequest(Request<?> request) {
• Span span = handler.handleSend(injector, request);
• span.remoteEndpoint(
• Endpoint.newBuilder().serviceName(request.getServiceName()).build());
• span.tag("aws.service_name", request.getServiceName());

I wonder if we should create a separate HttpClientParser to handle this stuff. I think because this is RPC over HTTP we should probably just make the Handler own this logic, and allow extending, for things like Dynamo and SQS with ClientParsers

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

[devinsba]

Looks like XRay doesn't consider them http spans. If they are talking to an AWS resource it uses the aws block instead of the http one in their model

[devinsba]

I think, this is good enough for a first pass. I'd like to get it merged so I can start getting the DynamoDB work we are doing that leverages it out into open source also. And then update the XRay storage adaptor to build the aws block correctly from these tags.