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

Remove support for URLConnectionSender in Zipkin autoconfiguration (#43048) #44500

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

Conversation

thecooldrop
Copy link

As per title I have removed and adapted the code which relied on presence of URLConnectionSender from Zipkin. Details in referenced issue #43048

Copy link
Contributor

@nosan nosan left a comment

Choose a reason for hiding this comment

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

Thank you, @thecooldrop
I've left some comments for your consideration.

@@ -74,17 +57,6 @@ void shouldBackOffOnCustomBeans() {
});
}

@Test
void shouldUseCustomHttpEndpointSupplierFactory() {
Copy link
Contributor

@nosan nosan Mar 2, 2025

Choose a reason for hiding this comment

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

Instead of removing this test, I think it should be adjusted. The HttpEndpointSupplier is also used by the ZipkinHttpClientSender. You can remove the FilteredClassLoader(HttpClient.class), retrieve the ZipkinHttpClientSender from the context, and verify that it is configured to use CustomHttpEndpointSupplier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Author

Choose a reason for hiding this comment

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

Implemented

});
}

@Test
void shouldUseUrlConnectionSenderIfHttpClientIsNotAvailable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing this test entirely, you can adjust it to verify:

If HttpClient is not present, then the context should not have ZipkinHttpClientSender.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Author

Choose a reason for hiding this comment

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

Implemented.

});
}

@Test
void shouldUseUrlConnectionSenderIfHttpClientIsNotAvailable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

@@ -74,17 +57,6 @@ void shouldBackOffOnCustomBeans() {
});
}

@Test
void shouldUseCustomHttpEndpointSupplierFactory() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label Mar 5, 2025
…by removal of URLConnectionSender usages in codebase

Signed-off-by: Vanio Begic <vanio.begic123@gmail.com>
@thecooldrop
Copy link
Author

Hi @mhalbritter thanks for taking a look. I have implemented the requested changes.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 5, 2025
… the originally applied to UrlConnectionSender

Signed-off-by: Vanio Begic <vanio.begic123@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants