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

Fixing bug in RequestUtil.ProcessRequest #58

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Conversation

djluck
Copy link
Contributor

@djluck djluck commented Feb 26, 2019

Issue: #57

Description of changes:
Don't access the trace context in RequestUtil.ProcessRequest when tracing is disabled. See for more information.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Don't access the trace context in `RequestUtil.ProcessRequest` when tracing is disabled. See aws#57 for more information.
@yogiraj07
Copy link
Contributor

Hi @djluck ,
Can we add unit test with tracing disabled on the recorder for sync and async call for HttpClient and HttpWebRequest.

…disabled. In the process, added disposal of HttpWebResponse/ HttpResponseMessage objects to avoid connection timeouts when sending multiple HTTP requests to the same endpoint.
@djluck
Copy link
Contributor Author

djluck commented Feb 26, 2019

@yogiraj07 done. I also fixed an issue that would see test HTTP requests time out (response objects were not being disposed, so service point connections were not freed).

… tracing is disabled + fixing up naming of tests to be consistent with existing naming style.
@yogiraj07 yogiraj07 merged commit 29bbc3d into aws:master Feb 27, 2019
@yogiraj07
Copy link
Contributor

Hi @djluck ,
Thank you for the PR.

@djluck
Copy link
Contributor Author

djluck commented Feb 27, 2019

@yogiraj07 thanks for merging. When can I expect the latest release to be pushed out to nuget?

@yogiraj07
Copy link
Contributor

@djluck , is the fix blocking you? We usually maintain small release cycle.

@djluck
Copy link
Contributor Author

djluck commented Feb 27, 2019

It is unfortunately (local dev does not work with tracing enabled OR disabled). There seems to be no workaround that I can think of.

@yogiraj07
Copy link
Contributor

@djluck ,

Can you please let me know your usecase ? What problem you are facing for local development? Will this PR fix the issue or not?

@djluck
Copy link
Contributor Author

djluck commented Feb 27, 2019

I am working on an ASP.NET core application. It has two distinct environments: localdev (i.e. my local machine which does not have an X-Ray trace collection daemon) and lambda (has X-Ray). When running the integration tests locally, all traced HTTP calls fail with the exception outlined in the issue I filed. The reason for this is that some of the instrumented calls happen during test verification- outside the scope of a trace segment.
My solution will be to disable x-ray tracing locally and then enable it when running the application in AWS Lambda, so yes- this PR will fix my issue.

@djluck
Copy link
Contributor Author

djluck commented Feb 27, 2019

I have been able to work around the issue for now. Thanks for your help @yogiraj07, I can wait for your normal release cycle.

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

Successfully merging this pull request may close these issues.

2 participants