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

Make ZipkinHttpClientSender the default BytesMessageSender #42589

Closed
mhalbritter opened this issue Oct 10, 2024 · 10 comments · May be fixed by #43085
Closed

Make ZipkinHttpClientSender the default BytesMessageSender #42589

mhalbritter opened this issue Oct 10, 2024 · 10 comments · May be fixed by #43085
Assignees
Labels
status: superseded An issue that has been superseded by another

Comments

@mhalbritter
Copy link
Contributor

mhalbritter commented Oct 10, 2024

Right now, org.springframework.boot.actuate.autoconfigure.tracing.zipkin.ZipkinConfigurations.SenderConfiguration imports UrlConnectionSenderConfiguration before HttpClientSenderConfiguration. We should switch that around. Also we should think about removing support for URLConnectionSender altogether.

The ZipkinRestTemplateSender and ZipkinWebClientSender will be removed in Boot 3.5.0, too.

@mhalbritter mhalbritter added the type: enhancement A general enhancement label Oct 10, 2024
@mhalbritter mhalbritter added this to the 3.5.x milestone Oct 10, 2024
@wickdynex
Copy link
Contributor

Follow-Up Questions

Hi @mhalbritter,

I have a few follow-up questions regarding this issue:

  1. Develop branch: u didn't mentioned which branch would apply this modification, whether main or 3.2.x or someone else?🤔
  2. Proposed Steps: to switch the UrlConnectionSenderConfiguration and HttpClientSenderConfiguration, I supposed only to switch the import order, after switching, it may look like this:
	@Configuration(proxyBeanMethods = false) 
	@Import({ 
                        HttpClientSenderConfiguration.class,
			WebClientSenderConfiguration.class,      
			RestTemplateSenderConfiguration.class,   
			UrlConnectionSenderConfiguration.class
	})

does it works? Need I create a branch and apply this? What's your opinion?🥺
3. Future Plans: have you also the team haved a meeting about whether removing this URLConnectionSender @bean? At the same time, should I also remove the code which depending on these components?🤨
4. Assigned Task: have u already assigned this task to a person but not mentioned in this #42589 issue? 🥸

There is one thing I must clarify, I'm a green hand in spring-boot also java, did't participate in any huge program. If there is any mistake I've made, plz pardon me.😭❤️
I'm looking forward to you reply, thanks for answering my question.🥳

@philwebb
Copy link
Member

Develop branch: u didn't mentioned which branch would apply this modification, whether main or 3.2.x or someone else?🤔

The milestone target (top right of the issue) is set to 3.5.x for this one. This means we want to do it for the 3.5 release, which will be on main once we've branched. We usually branch about a month after the 3.4.0 release.

Assigned Task: have u already assigned this task to a person but not mentioned in this #42589 issue? 🥸

No, the issue is currently unassigned. We change "Assignees" when someone actively starts working on an issue.

I'll leave the other questions for @mhalbritter to answer.

@wickdynex
Copy link
Contributor

The milestone target (top right of the issue) is set to 3.5.x for this one. This means we want to do it for the 3.5 release, which will be on main once we've branched. We usually branch about a month after the 3.4.0 release.

It means I can't fix this issue before 3.5.x branched? Am I misunderstanding your meaning?🤔 So could you plz explain the meaning of opening this issue to me? Maybe I need to learn the GitHub basic usage of issues🥺, thank u.

No, the issue is currently unassigned. We change "Assignees" when someone actively starts working on an issue.

But now we didn't branch the 3.4.x, so it means this issue can't assign to anyone?

@philwebb
Copy link
Member

philwebb commented Nov 1, 2024

It means I can't fix this issue before 3.5.x branched? Am I misunderstanding your meaning?🤔 So could you plz explain the meaning of opening this issue to me?

You're welcome to take a look at the issue and fix it on main, but we won't be able to really review or merge it until we create a 3.4.x branch.

But now we didn't branch the 3.4.x, so it means this issue can't assign to anyone?

No, the issue can be assigned at any time. You're also welcome to submit a pull-request on any unassigned issue. Our wrokflow is that we assign an issue when we know somebody is actively working on it. This aims to prevent duplicated effort.

@wickdynex
Copy link
Contributor

It means I can't fix this issue before 3.5.x branched? Am I misunderstanding your meaning?🤔 So could you plz explain the meaning of opening this issue to me?

You're welcome to take a look at the issue and fix it on main, but we won't be able to really review or merge it until we create a 3.4.x branch.

But now we didn't branch the 3.4.x, so it means this issue can't assign to anyone?

No, the issue can be assigned at any time. You're also welcome to submit a pull-request on any unassigned issue. Our wrokflow is that we assign an issue when we know somebody is actively working on it. This aims to prevent duplicated effort.

Appreciate for your reply😇. There's another question:

if I open a PR to fix the bug on main branch, after you and your team create a new branch like 3.4.x branch, how could you apply the PR in the new branch? By cherry-pick or any operate else🤨?

And there are also the rest of the questions in my previous comment, Im looking forward to the reply😃. Anyway, thank u again.

@philwebb
Copy link
Member

philwebb commented Nov 1, 2024

how could you apply the PR in the new branch? By cherry-pick or any operate else

We can rebase the PR. That's a usual part of our process.

@mhalbritter
Copy link
Contributor Author

mhalbritter commented Nov 4, 2024

2. Proposed Steps: to switch the UrlConnectionSenderConfiguration and HttpClientSenderConfiguration, I supposed only to switch the import order, after switching, it may look like this:

Yes, this works.

  1. Future Plans: have you also the team haved a meeting about whether removing this URLConnectionSender @bean? At the same time, should I also remove the code which depending on these components?

I'd remove WebClientSenderConfiguration, RestTemplateSenderConfiguration and UrlConnectionSenderConfiguration altogether. This only leaves HttpClientSenderConfiguration, which makes the ZipkinHttpClientSender the default Zipkin sender. But we should do this in a separate issue.

@wickdynex Do you want to work on that issue? If so, i'd assign it to you.

@wickdynex
Copy link
Contributor

  1. Proposed Steps: to switch the UrlConnectionSenderConfiguration and HttpClientSenderConfiguration, I supposed only to switch the import order, after switching, it may look like this:

Yes, this works.

  1. Future Plans: have you also the team haved a meeting about whether removing this URLConnectionSender @bean? At the same time, should I also remove the code which depending on these components?

I'd remove WebClientSenderConfiguration, RestTemplateSenderConfiguration and UrlConnectionSenderConfiguration altogether. This only leaves HttpClientSenderConfiguration, which makes the ZipkinHttpClientSender the default Zipkin sender. But we should do this in a separate issue.

@wickdynex Do you want to work on that issue? If so, i'd assign it to you.

It's my pleasure to have a chance to work on this issue, I will try my best to achieve this target. But I'm new in this project🥺, do you have some advice or idea for deleting these configurations? You can have a try to assign this issue to my, but in case to avoid that I mess it up, I will ask and also comments a lot. Pardon me🥰.

Steps below, and you can point out the mistakes :

  1. remove all the configuration class in ZipkinConfigurations.
  2. locate the position where use the @Bean like urlConnectionSender, and delete or replace it to @Bean httpClientSender.
  3. use unit test to check if it works, if needed, I need to write more test cases.
  4. update the document and the comment in the modified files.

Am I right? Looking for your reply and modifications❤️.

@mhalbritter
Copy link
Contributor Author

mhalbritter commented Nov 6, 2024

In the scope of this issue, you should just reorder the configurations, like you said in #42589 (comment). And I guess some tests needs to be adapted.

For the removal of ZipkinWebClientSender and ZipkinRestTemplateSender I've created #43047.

For the removal of URLConnectionSender I've created #43048.

@mhalbritter
Copy link
Contributor Author

Closing in favor of #43085.

@mhalbritter mhalbritter closed this as not planned Won't fix, can't repro, duplicate, stale Nov 19, 2024
@mhalbritter mhalbritter added status: superseded An issue that has been superseded by another and removed type: enhancement A general enhancement labels Nov 19, 2024
@mhalbritter mhalbritter removed this from the 3.5.x milestone Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
3 participants