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

nrawssdk-v2: fix usage examples of nrawssdk.AppendMiddlewares #599

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Meroje
Copy link

@Meroje Meroje commented Nov 4, 2022

Links

Details

When testing the nrawssdk-v2 integration along with nrlambda I found out that the provided method of injecting the middleware to APIOptions does not work (no trace is seen for any aws call), what worked was to use the optFns param.

In this PR I'm only changing the documentation and example to reflect what has been working, but maybe you'd want to expand on it to add an exported function that can be used as an optFn ?

Also I could not find how to make the test fail as I'm not familiar enough with the internals.

@CLAassistant
Copy link

CLAassistant commented Nov 4, 2022

CLA assistant check
All committers have signed the CLA.

@iamemilio iamemilio self-requested a review November 15, 2022 18:51
@iamemilio iamemilio self-assigned this Nov 15, 2022
@iamemilio
Copy link
Contributor

iamemilio commented Jan 10, 2023

@Meroje Hi, I'm taking a look at this and was wondering what this really changes other than when nrawssdk.AppendMiddlewares(&awsConfig.APIOptions, nil) is invoked. I can see that it's passing the unit test, so I'm not worried about the functionality. I am just not sure why we are changing the usage pattern on users if the outcome is the same.

@Meroje
Copy link
Author

Meroje commented Jan 10, 2023

I found that the previous method had no effect and I would never get any telemetry from aws sdk.

@iamemilio
Copy link
Contributor

Oh ok, that's a good reason haha. Ok, I'll verify this and see if we can land it in an upcoming release.

@Ak-x
Copy link

Ak-x commented Jul 25, 2023

@Meroje are you able to sign the CLA here so that we can proceed?

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

Successfully merging this pull request may close these issues.

None yet

4 participants