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

fix(AutoCollectHttpRequests): Http requests aren't collect if we call dispose+setup after http.createServer #947

Merged
merged 4 commits into from
Apr 15, 2022
Merged

fix(AutoCollectHttpRequests): Http requests aren't collect if we call dispose+setup after http.createServer #947

merged 4 commits into from
Apr 15, 2022

Conversation

Apokalypt
Copy link

- What is the problem?

Currently, every time an instance of AutoCollectHttpRequests is created, we overwrite the http(s).createServer function to add a handler to track HTTP(S) requests. As a result, if we call the constructor several times we end up with several handlers that can duplicate data.
The second problem is when we initialize the library once with setup() and then create the server with http(s).createServer. Here we track the requests correctly but if, for X reasons, we call dispose() and then setup(), the HTTP requests will not be tracked anymore because the code waits for a call to createServer to add the instance handler (despite the fact that the instance created at the first setup call already had one in place)

- How is it corrected?

As the code allows to have only one instance of AutoCollectHttpRequests at the same time ( stored in the static variable INSTANCE ), it is not necessary to modify the code of http(s).createServer each time the constructor is called. It is necessary to do it only once and in the added handler, we call a method of the instance stored in the static variable to register the request.
Thus, after the call to dispose() the registrations will stop but will resume after the call to setup despite the change of instance.


The fix is not optimized. In fact, this kind of handler should not be added when calling a constructor but rather at the very beginning of the library to optimize the chances that the library is well instantiated by the developers. However, after a discussion with @hectorhdzg ( #945 ) it turns out that this part of the code is going to change soon, so I didn't want to dwell on a code that will soon be completely overwritten.

…ed dispose and setup after http.createServer
@ghost
Copy link

ghost commented Apr 13, 2022

CLA assistant check
All CLA requirements met.

@Apokalypt
Copy link
Author

Apokalypt commented Apr 15, 2022

I don't understand why tests failed... They fail on a part that I did not modify and on top of that, they pass in local setup...

EDIT : they pass with no changes XD

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution

@hectorhdzg hectorhdzg merged commit 5c72f4e into microsoft:develop Apr 15, 2022
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.

None yet

2 participants