Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contrib/aws/{aws-sdk-go/aws, aws-sdk-go-v2/aws}: add context example #1504

Merged
merged 12 commits into from
Nov 28, 2023

Conversation

mackjmr
Copy link
Member

@mackjmr mackjmr commented Oct 7, 2022

This PR adds an example of passing context when making calls through the aws sdk. This allows the aws request spans to inherit their parent from context. This came up in APMS-7932.

This PR adds an example of passing context when making calls through the aws sdk. This allows the aws request spans to inherit their parent from context.
This came up in APMS-7932.
@mackjmr mackjmr requested a review from a team October 7, 2022 11:43
@mackjmr
Copy link
Member Author

mackjmr commented Oct 7, 2022

Not sure if an example should warrant an entry to go.mod ? (for github.com/aws/aws-sdk-go-v2/feature/s3/manager and github.com/aws/aws-sdk-go-v2/service/s3)
Perhaps I could add it in the package godoc instead so there is no need to update go.mod ?

@ajgajg1134 ajgajg1134 added this to the v1.44.0 milestone Oct 7, 2022
@ajgajg1134
Copy link
Contributor

Not sure if an example should warrant an entry to go.mod ? (for github.com/aws/aws-sdk-go-v2/feature/s3/manager and github.com/aws/aws-sdk-go-v2/service/s3) Perhaps I could add it in the package godoc instead so there is no need to update go.mod ?

We actually just had a discussion about this due to an issue caused by an example added for a different contrib package that brought in a significant number of go.mod changes/upgrades which forced those changes on our users and broke their code. So I'm thinking the easiest thing here would be to move the changes into go doc? It's not-ideal but I don't know a better approach

@mackjmr
Copy link
Member Author

mackjmr commented Oct 21, 2022

Moved the example into the package godoc @ajgajg1134 👍

@ajgajg1134
Copy link
Contributor

Thanks for moving the package godoc, one last possible request :P Could we move the package godoc into a separate docs file? doc.go perhaps? (e.g. ddtrace/tracer has a doc.go file)

@mackjmr
Copy link
Member Author

mackjmr commented Oct 28, 2022

Could we move the package godoc into a separate docs file? doc.go perhaps?

True, it's much cleaner that way @ajgajg1134! Updated :)

@ajgajg1134 ajgajg1134 modified the milestones: v1.44.0, v1.45.0 Nov 21, 2022
@Julio-Guerra Julio-Guerra modified the milestones: v1.45.0, Triage Dec 13, 2022
@ahmed-mez ahmed-mez added the apm:ecosystem contrib/* related feature requests or bugs label Aug 17, 2023
@katiehockman
Copy link
Contributor

@mackjmr do you still want to work on this? If so, there are some merge conflicts to resolve first.
Thanks for the contribution and the work you put in regardless!

contrib/aws/aws-sdk-go/aws/example_test.go Outdated Show resolved Hide resolved
contrib/aws/aws-sdk-go-v2/aws/doc.go Outdated Show resolved Hide resolved
@mackjmr
Copy link
Member Author

mackjmr commented Sep 26, 2023

@katiehockman I've resolved the conflicts :) I think it would still be good to get this example in, as it came up in an escalation.

@pr-commenter
Copy link

pr-commenter bot commented Sep 26, 2023

Benchmarks

Benchmark execution time: 2023-09-28 18:31:03

Comparing candidate commit bec66de in PR branch mackjmr/add-aws-sdk-go-context-example with baseline commit 04c819d in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics.

Comment on lines +6 to +73
// Package aws provides functions to trace aws/aws-sdk-go-v2 (https://github.com/aws/aws-sdk-go-v2).
//
// Usage Example:
//
// import (
// "context"
// "log"
// "os"
//
// "github.com/aws/aws-sdk-go-v2/aws"
// awscfg "github.com/aws/aws-sdk-go-v2/config"
// "github.com/aws/aws-sdk-go-v2/feature/s3/manager"
// "github.com/aws/aws-sdk-go-v2/service/s3"
// "github.com/aws/aws-sdk-go-v2/service/sqs"
//
// awstrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/aws/aws-sdk-go-v2/aws"
// "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
// "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
// )
//
// func Example() {
// awsCfg, err := awscfg.LoadDefaultConfig(context.Background())
// if err != nil {
// log.Fatalf(err.Error())
// }
// awstrace.AppendMiddleware(&awsCfg)
// sqsClient := sqs.NewFromConfig(awsCfg)
// sqsClient.ListQueues(context.Background(), &sqs.ListQueuesInput{})
// }
//
// // An example of the aws span inheriting a parent span from context.
// func Example_context() {
// cfg, err := awscfg.LoadDefaultConfig(context.TODO(), awscfg.WithRegion("us-west-2"))
// if err != nil {
// log.Fatalf("error: %v", err)
// }
// awstrace.AppendMiddleware(&cfg)
// client := s3.NewFromConfig(cfg)
// uploader := manager.NewUploader(client)
//
// // Create a root span.
// span, ctx := tracer.StartSpanFromContext(context.Background(), "parent.request",
// tracer.SpanType(ext.SpanTypeWeb),
// tracer.ServiceName("web"),
// tracer.ResourceName("/upload"),
// )
// defer span.Finish()
//
// // Open image file.
// filename := "my_image.png"
// file, err := os.Open(filename)
// if err != nil {
// log.Fatalf("error: %v", err)
// }
// defer file.Close()
//
// uploadParams := &s3.PutObjectInput{
// Bucket: aws.String("my_bucket"),
// Key: aws.String(filename),
// Body: file,
// ContentType: aws.String("image/png"),
// }
// // Inherit parent span from context.
// _, err = uploader.Upload(ctx, uploadParams)
// if err != nil {
// log.Fatalf("error: %v", err)
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is just a copy of the example we already have in example_test, then I'm not sure it adds too much extra value. I'm not sure if @ajgajg1134 was suggesting this during his last comment, or if he was suggesting something else...
I'd personally just remove this but keep the example code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func Example is a copy from example_test, but func Example_context was added, which is an example that demonstrate how to pass context.

The reason we moved all of this out of from example_test is to avoid adding entries to the go.mod that are necessary for func Example_context (for github.com/aws/aws-sdk-go-v2/feature/s3/manager and github.com/aws/aws-sdk-go-v2/service/s3)

@katiehockman katiehockman enabled auto-merge (squash) November 28, 2023 20:52
@katiehockman katiehockman merged commit ecfe10d into main Nov 28, 2023
51 checks passed
@katiehockman katiehockman deleted the mackjmr/add-aws-sdk-go-context-example branch November 28, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants