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

Adds commons module for apache httpclient #7914

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

anuragagarwal561994
Copy link
Contributor

@anuragagarwal561994 anuragagarwal561994 commented Feb 26, 2023

This MR separates out common code for apache http-client in 2 separate modules:

  • commons - this is to be used by all library versions
  • commons-4.0 - this is to be used by 4.x instrumentations

The code between the different versions of library instrumentations are repeated which makes it difficult to propagate a change in different versions as we have to modify multiple times.

Note that these modules are not being used currently, but next MRs will subsequently use them.

The idea is that we create OtelHttpRequest, OtelHttpResponse, OtelHttpContext interfaces which the particular version of library instrumentation implements.

Now since we have transformed these as our internal entities, the instrumentation code has to be written mostly once all the conversion between our internal entities and the instrumentation is being communicated by these classes.

Post this change 3 PRs will be created:

  • migrating apache httpclient 5 to use commons module
  • migrating apache httpclient 4.0 to use commons-4.0 module
  • migrating apache async httpclient 4.1 to use commons-4.0 module

@trask
Copy link
Member

trask commented Feb 27, 2023

migrating apache httpclient 5 to use commons module
migrating apache httpclient 4.0 to use commons-4.0 module
migrating apache async httpclient 4.1 to use commons-4.0 module

Go ahead and add these to this PR, it will help to see them together.

@anuragagarwal561994
Copy link
Contributor Author

@trask this PR seems to be ready from my side with 4.0, 4.1 and 5.0 instrumentations using the commons & commons-4.0

@anuragagarwal561994
Copy link
Contributor Author

Note that in httpclient 4.1, I have for now reverted the change

#6503

The issue here is that HttpHost is present in httpcore library while different httpclient libraries interacts with httpcore.

Now as stated in #7674

HttpHost doesn't have getAddress() method until 4.3 in httpcore.

This becomes difficult to manage and change here, either we will need to have a separate implementation for the same.
But I guess we can support the same from httpclient 5.x this socket address.

@anuragagarwal561994
Copy link
Contributor Author

Rethought upon this and found a way to include the above change, so nothing reverted.

@anuragagarwal561994
Copy link
Contributor Author

We can also include 4.3 module as well to use common modules but at present it is a library.

Hence to do so we will have to subdivide our commons and commons-4.0 into library vs agent.

This required additional effort which I can cover in another MR.

As my original MR will trigger otherwise a lot of merge conflicts.

@anuragagarwal561994
Copy link
Contributor Author

@trask let me know if anything else to be done in this MR

@anuragagarwal561994
Copy link
Contributor Author

@trask did you get time check this?

@trask
Copy link
Member

trask commented Mar 8, 2023

@anuragagarwal561994 sorry I haven't had time

@anuragagarwal561994
Copy link
Contributor Author

anuragagarwal561994 commented Mar 12, 2023

@trask can we look into this. I have simplified the changes it should take half an hour to 1 hour to review, if you check commit by commit.

@trask
Copy link
Member

trask commented Apr 2, 2023

@anuragagarwal561994 sorry, the project reviewers are stretched pretty thin, and so it's hard to review large refactoring changes like this

is your goal here just refactoring, or do you have a bug/enhancement you are trying to make?

if you have a bug/enhancement you are trying to make, it might be better to make the minimal changes needed to implement that, without trying to do a big refactoring

@anuragagarwal561994
Copy link
Contributor Author

@trask the refactoring goal is actually needed because currently the http client modules have almost similar code and logic with minor changes.

The refactoring here does just clubs this common logic for any new thing to be integrated better.

While I was adding this new feature adding correct request and response length there were many new classes and code pieces involved with the code change and I had to repeat it thrice in 5.x, 4.1-async and 4.x

Now a small change here or there was breaking the other instrumentation or test cases and several changes were involved which I had to repeat and fix. There were many iterations done to arrive at the correct approach of how the data is to be retrieved from the library and I had to always make those changes three times.

Hence after refactoring the instrumentation and putting the common logic together really helped me with modifying the changes and thinking of different strategies faster and hence that feature ended up getting connected with this refactoring and trust me when I say this I had to redesign and reimplement a lot of times after my initial draft and everytime I had to make changes for multiple modules.

I understand your concern that it is a big change, but I tried in the best way to simply it.

That is why I had initially separated only the common module and asked to further integrate each module separately as different MRs to reduce the scope of the problem.

If that is okay for you, I can further break the changes as I had earlier suggested.

Changes in teat cases are not done at all so at least functionally the code should be correct.

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