contrib/aws/aws-sdk-go: add WithErrorCheck option #1682

delca85 · 2023-01-22T21:12:40Z

What does this PR do?

It adds an option to aws/aws-sdk-go tracer. This option is called WithErrorCheck and it specifies a function which determines whether the passed error should be marked as an error. The fn is called whenever an aws operation finishes with an error.

Motivation

This applies the same logic introduced by #1315 on sql tracer.
We need this otherwise DD raises an issue for every kind of error.

Describe how to test/QA your changes

Reviewer's Checklist

If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

WithErrorCheck specifies a function which determines whether the passed error should be marked as an error. The fn is called whenever an aws operation finishes with an error.

johanneswuerbach · 2023-02-08T13:17:11Z

@knusbaum sorry for the direct ping, but any chance to get this or something similar merge? This is currently polluting our error tracking :-/

johanneswuerbach · 2023-02-22T08:54:27Z

@katiehockman any chance this could also be added to next milestone?

dianashevchenko · 2023-03-15T12:25:38Z

contrib/aws/aws-sdk-go/aws/example_test.go

+			cfg := aws.NewConfig().WithRegion("us-west-2")
+			sess := session.Must(session.NewSession(cfg))


Suggested change

cfg := aws.NewConfig().WithRegion("us-west-2")

sess := session.Must(session.NewSession(cfg))

sess := session.Must(session.NewSession( aws.NewConfig().WithRegion("us-west-2")))

dianashevchenko · 2023-03-15T12:25:55Z

contrib/aws/aws-sdk-go/aws/example_test.go

+			spans := mt.FinishedSpans()
+			assert.True(t, len(spans) > 0)
+


This part is failing in the tests

As written below, I ran the contrib test with ./test.sh --contrib and every test under contrib/aws/aws-sdk-go/aws succeeded.

dianashevchenko · 2023-03-15T12:26:50Z

contrib/aws/aws-sdk-go/aws/example_test.go

+			s := spans[len(spans)-1]
+			assert.Equal(t, errExist, s.Tag(ext.Error) != nil)


Suggested change

s := spans[len(spans)-1]

assert.Equal(t, errExist, s.Tag(ext.Error) != nil)

assert.Equal(t, errExist, spans[0].Tag(ext.Error) != nil)

dianashevchenko · 2023-03-15T12:28:33Z

contrib/aws/aws-sdk-go/aws/option.go

+
+// WithErrorCheck specifies a function fn which determines whether the passed
+// error should be marked as an error. The fn is called whenever an aws operation
+// finishes with an error


Suggested change

// finishes with an error

// finishes with an error.

dianashevchenko · 2023-03-15T12:37:20Z

contrib/aws/aws-sdk-go/aws/example_test.go

+
+func TestWithErrorCheck(t *testing.T) {
+	testOpts := func(errExist bool, opts ...awstrace.Option) func(t *testing.T) {
+		return func(t *testing.T) {
+			mt := mocktracer.Start()
+			defer mt.Stop()
+


I believe this test belongs in aws_test.go.

dianashevchenko · 2023-03-15T12:47:51Z

contrib/aws/aws-sdk-go/aws/example_test.go

+	t.Run("errcheck", testOpts(false, awstrace.WithErrorCheck(func(err error) bool {
+		return !strings.Contains(err.Error(), `InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records`)


The error that is actually received doesn't contain this text, so it will fail. For the test purposes, it's best to check for different content. NoCredentialProviders: no valid providers in chain. Deprecated.

Suggested change

t.Run("errcheck", testOpts(false, awstrace.WithErrorCheck(func(err error) bool {

return !strings.Contains(err.Error(), `InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records`)

t.Run("errcheck", testOpts(false, awstrace.WithErrorCheck(func(err error) bool {

return !strings.Contains(err.Error(), `NoCredentialProviders: no valid providers in chain`)

I ran the contrib test with ./test.sh --contrib and every test under contrib/aws/aws-sdk-go/aws succeeded.

Unfortunately, the workflow is still failing - link. Since the error I received signified the lack of credentials, it is possible that you have some environment variables set on your machine. I would recommend to rerun this test specifically and include a different error check.
I have changed the assertion to log the error:

assert.Equal(t, errExist, spans[0].Tag(ext.Error) != nil, fmt.Sprintf("error.message isn't nil: %s", spans[0].Tag(ext.Error)))

Here is the output of running a test on my machine.

Error Trace: /Users/diana.shevchenko/go/src/github.com/dd-trace-go/contrib/aws/aws-sdk-go/aws/aws_test.go:273 Error: Not equal: expected: false actual : true Test: TestWithErrorCheck/errcheck Messages: error.message isn't nil: NoCredentialProviders: no valid providers in chain. Deprecated. For verbose messaging see aws.Config.CredentialsChainVerboseErrors

Like I have mentioned previously, the error contains NoCredentialProviders

Thank you, I didn't think about some issues related to my local environment.
I pushed new code according to your suggestion.

dianashevchenko · 2023-03-15T12:52:02Z

contrib/aws/aws-sdk-go/aws/example_test.go

+			mt := mocktracer.Start()
+			defer mt.Stop()
+
+			cfg := aws.NewConfig().WithRegion("us-west-2")
+			sess := session.Must(session.NewSession(cfg))
+			sess = awstrace.WrapSession(sess, opts...)
+
+			s3api := s3.New(sess)
+			s3api.CreateBucket(&s3.CreateBucketInput{
+				Bucket: aws.String("some-bucket-name"),
+			})
+
+			spans := mt.FinishedSpans()
+			assert.True(t, len(spans) > 0)


One has to create a span before hand, an example of that can be found in other tests of aws_test.go.

root, ctx := tracer.StartSpanFromContext(context.Background(), "test") sess := session.Must(session.NewSession(aws.NewConfig().WithRegion("us-west-2"))) sess = WrapSession(sess, opts...) s3api := s3.New(sess) s3api.CreateBucketWithContext(ctx, &s3.CreateBucketInput{ Bucket: aws.String("some-bucket-name"), }) root.Finish()

I've added the creation of the span.

delca85 · 2023-04-18T09:38:05Z

@dianashevchenko is there still any problem with this PR? I saw it had been tagged as related to v1.49.0 release but then never merged.

…nflict resolution in tests

…ary updates

zarirhamza · 2023-05-12T15:09:20Z

LGTM

Just a minor change to only check the return boolean value in the tests. As we update AWS libraries, the exact string of the error will most likely change (which it did) and will break the test every time, so we future proofed it by only checking the boolean return value.

Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> Co-authored-by: Zarir Hamza <zarir.hamza@datadoghq.com>

Adds an WithErrorCheck function similar to #1682

contrib/aws/aws-sdk-go: add WithErrorCheck option

6f96744

WithErrorCheck specifies a function which determines whether the passed error should be marked as an error. The fn is called whenever an aws operation finishes with an error.

delca85 requested a review from a team January 22, 2023 21:12

Merge branch 'main' into v1-contrib-aws-error-with-check

f3ee7c2

dianashevchenko added this to the v1.49.0 milestone Mar 15, 2023

dianashevchenko added the apm:ecosystem contrib/* related feature requests or bugs label Mar 15, 2023

dianashevchenko requested changes Mar 15, 2023

View reviewed changes

delca85 and others added 2 commits March 15, 2023 15:01

contrib/aws/aws-sdk-go: add WithErrorCheck option

0b7e69f

Merge branch 'main' into v1-contrib-aws-error-with-check

2001420

delca85 requested a review from dianashevchenko April 28, 2023 06:52

Merge branch 'main' into v1-contrib-aws-error-with-check

708d72c

ajgajg1134 modified the milestones: v1.49.0, v1.51.0 May 12, 2023

zarirhamza added 2 commits May 12, 2023 10:34

contrib/aws/aws-sdk-go/aws: Fixed missing parentheses due to merge co…

1e7d08d

…nflict resolution in tests

contrib/aws/aws-sdk-go/aws: Future proof error tests against aws libr…

465e65b

…ary updates

ajgajg1134 approved these changes May 12, 2023

View reviewed changes

dianashevchenko approved these changes May 12, 2023

View reviewed changes

ajgajg1134 merged commit aff2b5a into DataDog:main May 12, 2023
9 checks passed

katiehockman pushed a commit that referenced this pull request Jun 6, 2023

contrib/aws/aws-sdk-go: add WithErrorCheck option (#1682)

2b4f0b6

Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> Co-authored-by: Zarir Hamza <zarir.hamza@datadoghq.com>

johanneswuerbach mentioned this pull request Jun 14, 2023

contrib/aws/aws-sdk-go-v2: add WithErrorCheck option #2042

Merged

2 tasks

katiehockman pushed a commit that referenced this pull request Jun 28, 2023

contrib/aws/aws-sdk-go-v2: add WithErrorCheck option (#2042)

0c67903

Adds an WithErrorCheck function similar to #1682

This was referenced Nov 28, 2023

contrib/aws-sdk-go: add WithErrorCheck for aws sdk (v1 & v2) #1870

Closed

contrib/aws/aws-sdk-go: Add WithErrorCheck option #1664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/aws/aws-sdk-go: add WithErrorCheck option #1682

contrib/aws/aws-sdk-go: add WithErrorCheck option #1682

delca85 commented Jan 22, 2023 •

edited

Loading

johanneswuerbach commented Feb 8, 2023

johanneswuerbach commented Feb 22, 2023

dianashevchenko Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

dianashevchenko Mar 15, 2023

delca85 Mar 15, 2023

delca85 commented Apr 18, 2023

zarirhamza commented May 12, 2023

		cfg := aws.NewConfig().WithRegion("us-west-2")
		sess := session.Must(session.NewSession(cfg))

		s := spans[len(spans)-1]
		assert.Equal(t, errExist, s.Tag(ext.Error) != nil)

	s := spans[len(spans)-1]
	assert.Equal(t, errExist, s.Tag(ext.Error) != nil)
	assert.Equal(t, errExist, spans[0].Tag(ext.Error) != nil)

		t.Run("errcheck", testOpts(false, awstrace.WithErrorCheck(func(err error) bool {
		return !strings.Contains(err.Error(), `InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records`)

contrib/aws/aws-sdk-go: add WithErrorCheck option #1682

contrib/aws/aws-sdk-go: add WithErrorCheck option #1682

Conversation

delca85 commented Jan 22, 2023 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

johanneswuerbach commented Feb 8, 2023

johanneswuerbach commented Feb 22, 2023

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

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

delca85 commented Apr 18, 2023

zarirhamza commented May 12, 2023

delca85 commented Jan 22, 2023 •

edited

Loading