-
Notifications
You must be signed in to change notification settings - Fork 41k
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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() { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
}); | ||
} | ||
|
||
@Test | ||
void shouldUseUrlConnectionSenderIfHttpClientIsNotAvailable() { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented.
}); | ||
} | ||
|
||
@Test | ||
void shouldUseUrlConnectionSenderIfHttpClientIsNotAvailable() { |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
…by removal of URLConnectionSender usages in codebase Signed-off-by: Vanio Begic <vanio.begic123@gmail.com>
Hi @mhalbritter thanks for taking a look. I have implemented the requested changes. |
… the originally applied to UrlConnectionSender Signed-off-by: Vanio Begic <vanio.begic123@gmail.com>
As per title I have removed and adapted the code which relied on presence of URLConnectionSender from Zipkin. Details in referenced issue #43048