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

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

Meroje
Copy link
Contributor

@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
Contributor 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?

@iamemilio iamemilio closed this Jul 29, 2024
@iamemilio iamemilio reopened this Jul 29, 2024
@nr-swilloughby nr-swilloughby changed the base branch from master to develop September 30, 2024 08:53
Copy link
Contributor

@nr-swilloughby nr-swilloughby left a comment

Choose a reason for hiding this comment

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

These changes are approved and we're reviewing what potential changes to the integration may be needed to better support AWS as highlighted by this PR.

@nr-swilloughby nr-swilloughby merged commit fe29069 into newrelic:develop Oct 10, 2024
54 checks passed
This was referenced Oct 10, 2024
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.

5 participants