aws sdk v2 instrumentation support

[bhautikpip]

Issue #, if available:

Description of changes:

Continuation of this PR: #297

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[srprash]

Looks good. Just some minor suggestions.
Thanks!

[srprash]

Can we change the method name from "ApendMiddlewares" to something that communicates AWS SDK v2 instrumentation clearly? For example "AddAWSV2Tracing" or something else.

[bhautikpip]

Done.

[srprash]

This little code block can be its own private method. Makes it a bit cleaner.

[bhautikpip]

Done.

[anuraaga]

Can you move this into another package? We found the current pattern bloats binary size so we should aim for a package for each instrumentation

[bhautikpip]

I have added the separate module for this PR but this change would require a lot of re working because currently we release the repository as a single package which includes instrumentation packages and SDK. Now, If we want to create a separate module for each instrumentation package then I think that would require moving things around and basically separate out the SDK and instrumentation packages. Once we do that then, we can manage separate release of core SDK and instrumentation packages. I am not able to visualize the release process in this scenario where we add separate instrumentation package at the root of this repository.

[anuraaga]

I was hoping having just a separate package, without a separate go.mod, would be enough for go to not include unneeded code in a downstream binary. Do you know if this is the case or not? With separate go.mod it obviously is but hoping just package separation is enough, go compiler does do dead code elimination - it doesn't seem to be very great at it but if a package boundary isn't enough to trigger it, then I'm not sure why the feature exists in the first place, I have a feeling it will work :P

[anuraaga]

Actually thinking more don't see a problem with the current PR adding a go.mod. What do you mean about the release process? Isn't it just cutting a tag in this repo?

We can't really move around the existing instrumentation without a major version bump so we'll hold off on that, but we may as well try to split better for new instrumentation.

[bhautikpip]

Basically, currently we are tagging the entire repository as released version. If we separate instrumentation package then It means some modules in the instrumentation packages relies on core xray package. So, I am not able to visualize how we are going to tag these packages without reworking the SDK. Ideally what we should do is to have separate module for core SDK and separate module for instrumentation and we should do the release of core SDK package and separate instrumentation packages rather than tagging an entire repository. If we take open telemetry contrib repo as an example here's how various released packages looks like: https://github.com/open-telemetry/opentelemetry-go-contrib/releases . I am not sure about this - #309 (comment) AFAIK In order to reduce the dependencies it has to be the module but I will dig more and update the thread.

[wangzlei]

@bhautikpip Any other concern if we just create separate go.mod for aws sdk v2 instrumentation, not change legacy instrumentations? We can update release process for aws sdk v2 instrumentation only.

[bhautikpip]

@wangzlei As I have mentioned it would require moving things around(mostly reworking of SDK) within SDK. Right now instrumentation packages and core SDK packages resides in xray dir. We probably have to separate out those components. Also, we have some packages at the root level so we will have to package those in with xray dir. @anuraaga and I had a discussion regarding the same so we end up deciding to have a separate package for aws v2 instrumentation and not a module right now to gain some advantage of dead code elimination. Also, this would require a major version release since this is a breaking change.

[callado4]

Is this root.Close(nil) needed again since there is already a defer root.Close(nil) above?

[bhautikpip]

It's not. good callout.

[bhautikpip]

Update: I am currently validating the dependency size after adding aws sdk instrumentation dependencies to see if the number is not huge once I can confirm I will merge this PR.

[bhautikpip]

@wangzlei tested below code (which has xray and awsv2 imports) to check for binary size and the size was 21.2 MB. I think this instrumentation does not add significant amount of overhead in size so should be good to merge.

package main

import (
	"context"
	"log"

	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/service/dynamodb"
	"github.com/aws/aws-xray-sdk-go/instrumentation/awsv2"
	"github.com/aws/aws-xray-sdk-go/xray"
)

func main() {
	ctx, root := xray.BeginSegment(context.TODO(), "AWSSDKV2_Dynamodb")
	defer root.Close(nil)
	cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion("us-west-2"))
	if err != nil {
		log.Fatalf("unable to load SDK config, %v", err)
	}
	// Instrumenting AWS SDK v2
	awsv2.AWSV2Instrumentor(&cfg.APIOptions)
	// Using the Config value, create the DynamoDB client
	svc := dynamodb.NewFromConfig(cfg)
	// Build the request with its input parameters
	_, err = svc.ListTables(ctx, &dynamodb.ListTablesInput{
		Limit: aws.Int32(5),
	})
	if err != nil {
		log.Fatalf("failed to list tables, %v", err)
	}
}