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

v3/integrations/nrawssdk-v2: Does not work with the v1.0.0+ releases of AWS SDK v2 #288

Closed
nc-wittj opened this issue Mar 29, 2021 · 19 comments · Fixed by #309, #328 or #337
Closed

v3/integrations/nrawssdk-v2: Does not work with the v1.0.0+ releases of AWS SDK v2 #288

nc-wittj opened this issue Mar 29, 2021 · 19 comments · Fixed by #309, #328 or #337
Assignees
Labels

Comments

@nc-wittj
Copy link
Contributor

Description

It seems the v1.0.0+ Go AWS v2 SDK had a few breaking changes since the beta releases, which the v3/integrations/nrawssdk-v2 package seems to be based on. Thus, we cannot use the NewRelic integration with our application using the latest AWS v2 SDK.

Steps to Reproduce

Use v3/integrations/nrawssdk-v2 with the latest AWS SDK v2 release. Notice errors when using go get:

$ go get github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2
go: downloading github.com/newrelic/go-agent/v3 v3.11.0
go: downloading github.com/newrelic/go-agent v3.11.0+incompatible
go: downloading github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2 v1.0.0
# github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2
../../../../../go/pkg/mod/github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2@v1.0.0/nrawssdk.go:12:24: undefined: aws.Request
../../../../../go/pkg/mod/github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2@v1.0.0/nrawssdk.go:23:22: undefined: aws.Request
../../../../../go/pkg/mod/github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2@v1.0.0/nrawssdk.go:76:35: undefined: aws.Handlers
../../../../../go/pkg/mod/github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2@v1.0.0/nrawssdk.go:77:30: undefined: aws.NamedHandler
../../../../../go/pkg/mod/github.com/newrelic/go-agent/v3/integrations/nrawssdk-v2@v1.0.0/nrawssdk.go:81:29: undefined: aws.NamedHandler

Expected Behavior

It works and does not have compile time errors.

NR Diag results

Your Environment

Reproduction case

Additional context

@nc-wittj nc-wittj added the bug label Mar 29, 2021
@RichVanderwal
Copy link
Contributor

Thanks, @nc-wittj ! We'll hand this to our Product Manager to help us prioritize this bug in our NR AWS SDK V2 integration.

@RichVanderwal
Copy link
Contributor

Also, I should mention that this is scheduled to be fixed in our April 2021 maintenance release. #250

@RichVanderwal RichVanderwal added this to the Apr Go agent release milestone Mar 29, 2021
@nc-wittj
Copy link
Contributor Author

@RichVanderwal cool, thanks!

@betabandido
Copy link

Any news on this? This is blocking us from using NR to monitor AWS requests.

@RichVanderwal
Copy link
Contributor

Hi @betabandido , we've looked at this today, but are unable to reproduce it so far. We have some ideas about what might be causing this for some people and not others, and we'll let you know as soon as we're able to make some progress.

@betabandido
Copy link

Hi @RichVanderwal -- let me know if I can help in any way so you can reproduce it.

@nc-wittj
Copy link
Contributor Author

@RichVanderwal It should be quite simple to reproduce. I put together a repo to help your team out. Hope this helps!

@RichVanderwal
Copy link
Contributor

Easily reproducible by rolling back to Go 1.15 or earlier.

@RichVanderwal
Copy link
Contributor

Thanks, @nc-wittj for the repo and explanation! We'll work to get a fix into this month's maintenance release.

@RichVanderwal
Copy link
Contributor

We're going to need to re-write this integration.

Version 0.25 of the AWS SDK V2 removed the shared client system, upon which our instrumentation depends. It was replaced with an alternate system described in this announcement from AWS. We’ll be reviewing new approaches to this. Alternatives include finding a new bottleneck in how their Amazon Smithy HTTP layers work, or following the lead of the OpenTelemetry for Go project and simply writing middleware for instrumentation.

@nc-wittj
Copy link
Contributor Author

@RichVanderwal Happy to help! Yeah, I figured it would need a re-write. 😞 Hopefully it's not too tricky, and you can get it in this month's maintenance release!

@RichVanderwal
Copy link
Contributor

Unfortunately, we won't be able to make it into this month's maintenance release. It's at the top of our list, though, and work is currently being done on this!

Question: our AWS SDK V1 instrumentation records the duration of the request to AWS. Very simple. We can do the same thing with AWS SDK V2. However, with the introduction of a five-stage middleware stack, it might make a lot of sense to record the entire time spent inside the stack, as well. Here's the picture from Amazon's documentation:

image

How much would it mean to you to have two segments instead of one -- an inner one that measures the HTTP request duration like our AWS SDK V1 instrumentation, and then another parent segment that records the whole SDK operation?

@nc-wittj
Copy link
Contributor Author

@RichVanderwal I synced with my team, and we don't think having an extra segment would be super helpful in general, as the bulk of the time should be the network call. There were also some concerns about it being confusing to have two segments. Although, re-reading your comment it sounds like there's some parent-child relationship between the segments? So if it's obvious the two segments are related, that solves that concern.

So, as long as it doesn't delay the fix, I think we'd be fine with two segments if it's obvious how they are related in the UI. But, if it'll take more development time, we'd prefer to have the feature with one segment. Hopefully that helps!

@RichVanderwal RichVanderwal linked a pull request May 7, 2021 that will close this issue
@RichVanderwal RichVanderwal self-assigned this May 24, 2021
@RichVanderwal
Copy link
Contributor

Fixed in the May Go Agent Release. We'd love to hear feedback from @nc-wittj , @betabandido, @R-NK, @zsrinivas, and @HenriBeck !

@HenriBeck
Copy link

@RichVanderwal I think the integration will work for us.

However, I do have one question regarding the usage: Is there any reason why it doesn't simply extract the newrelic transaction from the context.Context in the middleware?

At least how we used the AWS client previously we created a single client upfront and then called it multiple times in our application lifecycle.

@RichVanderwal
Copy link
Contributor

Hi @HenriBeck , when tracing through the Smithy middleware, the newrelic transaction was nowhere to be found in the context. A bit disappointing, we seemed to be handed different context than what my sample application was using.

It's worth double-checking that, though. Thanks for the immediate feedback!

@RichVanderwal RichVanderwal reopened this Jun 2, 2021
@HenriBeck
Copy link

@RichVanderwal from my testing today it seems to work for me to extract the transaction from the context with a minimal middleware like this:

func NewRelicMiddleware(stack *smithymiddle.Stack) error {
	return stack.Deserialize.Add(
		smithymiddle.DeserializeMiddlewareFunc(
			"NRDeserializeMiddleware",
			func(
				ctx context.Context,
				in smithymiddle.DeserializeInput,
				next smithymiddle.DeserializeHandler,
			) (out smithymiddle.DeserializeOutput, metadata smithymiddle.Metadata, err error) {
				smithyRequest := in.Request.(*smithyhttp.Request)
				txn := newrelic.FromContext(ctx)

				// The actual http.Request is inside the smithyhttp.Request
				httpRequest := smithyRequest.Request
				segment := newrelic.StartExternalSegment(txn, httpRequest)

				// Hand off execution to other middlewares and then perform the request
				out, metadata, err = next.HandleDeserialize(ctx, in)

				segment.End()
				return out, metadata, err
			},
		),
		smithymiddle.Before,
	)
}

I guess it also would be easy enough to just read from the context in case the txn on the middleware itself is nil just to keep this manual option also available

@nc-wittj
Copy link
Contributor Author

nc-wittj commented Jun 7, 2021

@RichVanderwal I implemented @HenriBeck's suggestion in this PR: #328

It seems to be working well, and is much easier to leverage in our serverless app. Let me know what you think.

@RichVanderwal RichVanderwal linked a pull request Jul 2, 2021 that will close this issue
@RichVanderwal
Copy link
Contributor

Releasing this soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment