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

Move JerseyServerMetricsAutoConfiguration to web package #14881

Closed
mbhave opened this issue Oct 17, 2018 · 7 comments
Closed

Move JerseyServerMetricsAutoConfiguration to web package #14881

mbhave opened this issue Oct 17, 2018 · 7 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: task A general task

Comments

@mbhave
Copy link
Contributor

mbhave commented Oct 17, 2018

It looks like it should live in org.springframework.boot.actuate.autoconfigure.metrics.web.

@mbhave
Copy link
Contributor Author

mbhave commented Oct 17, 2018

Placing on hold till #14860 is sorted.

@mbhave mbhave added type: enhancement A general enhancement type: task A general task and removed type: enhancement A general enhancement labels Oct 17, 2018
@mbhave mbhave added this to the 2.x milestone Oct 17, 2018
@nishantraut
Copy link
Contributor

nishantraut commented Oct 23, 2018

Hi @mbhave This looks simple moving class from old package to new mentioned above. Please let me know once this issue is removed from hold state. Will be happy to work on this. Thanks.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Apr 26, 2022
@wilkinsona
Copy link
Member

Flagging for team attention so that we can consider doing this in 3.0 without #14860.

@wilkinsona wilkinsona modified the milestones: 2.x, 3.0.x Apr 27, 2022
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Apr 27, 2022
@wilkinsona
Copy link
Member

This is blocked until we've reinstated support for Jersey which should be possible once 3.1 has been released.

The main Jersey auto-configuration is in org.springframework.boot.autoconfigure.jersey so the current location of JerseyServerMetricsAutoConfigurationorg.springframework.boot.actuate.autoconfigure.metrics.jersey – feels right to me. If we moved the metrics auto-configuration to org.springframework.boot.actuate.autoconfigure.metrics.web then I wonder if JerseyAutoConfiguration should move too.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label May 5, 2022
@mbhave
Copy link
Contributor Author

mbhave commented Jun 27, 2022

I think I might've seen that the actuator auto-configuration is under org.springframework.boot.actuate.autoconfigure.web.jersey when I logged this issue. That's also a bit odd because there's org.springframework.boot.actuate.autoconfigure.web.servlet which has MVC-related classes but could have jersey?

JerseyApplicationPath and DefaultJerseyApplicationPath are in org.springframework.boot.autoconfigure.web.servlet which also seems inconsistent.

@wilkinsona
Copy link
Member

Thanks, @mbhave.

org.springframework.boot.actuate.autoconfigure.web.servlet is named after org.springframework.web.servlet which is the root package for Spring MVC. As such, I don't think Jersey-related code belongs in a web.servlet package.

org.springframework.boot.actuate.autoconfigure.web.jersey is the auto-configuration for the actuator's web endpoint infrastructure when running on top of Jersey. There are .servlet and .reactive packages here too for the MVC and WebFlux support. We also have org.springframework.boot.actuate.autoconfigure.endpoint.web.jersey (and .servlet and .reactive variants as well). We only have one class in each of these packages. The distinction between .endpoint.web.<framework> and .web.<framework> isn't very clear to me. It's all code for running Actuator on top of a particular framework. I wonder if they could be combined.

The location of JerseyApplicationPath and DefaultJerseyApplicationPath does seem inconsistent. I think they should both be in the same package as the rest of the Jersey auto-configuration. That's org.springframework.boot.autoconfigure.jersey at the moment.

It looks to me like that are two, somewhat separate packaging questions here:

  1. Do we need the separate org.springframework.boot.actuate.autoconfigure.web.<framework> and org.springframework.boot.actuate.autoconfigure.endpoint.web.<framework> packages?
  2. Is org.springframework.boot.autoconfigure.jersey the right package for the Jersey auto-configuration?

The answers to these will then have a knock-on effect on the two groups of Jersey-related packages:

  1. Code in support of using and observing Jersey in an application
  2. Code in support of running Actuator on top of Jersey

@bclozel
Copy link
Member

bclozel commented Jun 29, 2022

While discussing this issue, we've found new candidates for relocation/renaming.

  • we are wondering if we should split the existing org.springframework.boot.actuate.autoconfigure.web package and create a new org.springframework.boot.actuate.autoconfigure.infrastructure package
  • we also noticed ServletEndpointManagementContextConfiguration in org.springframework.boot.actuate.autoconfigure.endpoint.web; it is using the ManagementContextConfiguration and referring to both Spring MVC and Jersey

@wilkinsona wilkinsona modified the milestones: 3.0.x, 3.x Oct 3, 2022
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review labels Oct 13, 2022
@wilkinsona wilkinsona removed this from the 3.x milestone Oct 13, 2022
@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: task A general task
Projects
None yet
Development

No branches or pull requests

4 participants