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 WebServers' started log messages more consistent #36149

Closed
wilkinsona opened this issue Jun 30, 2023 · 11 comments
Closed

Make WebServers' started log messages more consistent #36149

wilkinsona opened this issue Jun 30, 2023 · 11 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@wilkinsona
Copy link
Member

Working on #36103 showed that there are some inconsistencies in how we log the Servlet WebServer's context path:

  • Jetty logs '/' by default
  • Tomcat logs '' by default
  • Undertow logs nothing by default

There are also some similar inconsistencies on the reactive side where there is no (configurable) context path:

  • Jetty logs '/'
  • Tomcat logs ''
  • Undertow logs nothing
  • Netty logs nothing

We should try to make things as consistent as possible, removing as many of the optional regex groups as possible from the tests added in #36103.

@wilkinsona
Copy link
Member Author

For Servlet WebServers I think we should always log something, even with the default empty context path. It would also be good to log '' or '/' consistently for this case.

For reactive WebServers I think it probably makes sense not to log anything at all as there isn't really a context path in this case. Technically, you could configure one for some of the servers, for example with a TomcatContextCustomizer when using Tomcat, but I'm not sure we should account for that in the log messages. I'd like to see what the rest of the team thinks about this though.

@wilkinsona wilkinsona added type: enhancement A general enhancement for: team-attention An issue we'd like other members of the team to review labels Jun 30, 2023
@wilkinsona wilkinsona added this to the 3.2.x milestone Jun 30, 2023
@philwebb
Copy link
Member

philwebb commented Jul 5, 2023

We're going with "Tomcat started on ports 8080 (http) with context path '/'" for servlets and "Tomcat started on ports 8080 (http)" for reactive.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jul 5, 2023
@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.x Sep 7, 2023
@emileplas
Copy link

@wilkinsona : if you wouldn't mind, I would try to have a go at this PR.

@vicaba
Copy link

vicaba commented Dec 11, 2023

Hi @wilkinsona, @emileplas , I am trying to start contributing to spring boot. I thought this could be a great issue to start with as it seems easy. If @emileplas hasn't done it yet, I think I can start.

First time contributing to an open source project, so I may need a little help.

From #36103 , I have identified that the change is needed in the following files:

  • spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyWebServer.java
  • spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/netty/NettyWebServer.java
  • spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatWebServer.java
  • spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/UndertowWebServer.java

I have also identified the code that needs to be changed. At least the "end" code, I guess I will have to reverse engineer things to see where the issue comes form. However, I would like to know which branch I should be working on (base my PR branch on), because now I'm looking at the main branch and I'm not sure that this is the correct one.

@philwebb
Copy link
Member

Hi @vicaba, thanks for offering to pick this one up if @emileplas doesn't mind (let's give him a few days to respond).

I would like to know which branch I should be working on

We'd consider this one an enhancement so it only needs to be applied to the main branch.

@emileplas
Copy link

@vicaba go ahead.

@vicaba
Copy link

vicaba commented Dec 13, 2023

I have some news on this issue.

  • I have found several smoke tests where I can try and visually check the changes. However, there is no smoke test for netty. These are the smoke tests:

    • spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-jetty/src/main/java/smoketest/jetty/SampleJettyApplication.java
    • There is no spring-boot-smoke-test-netty?
    • spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-tomcat/src/main/java/smoketest/tomcat/SampleTomcatApplication.java
    • spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-undertow/src/main/java/smoketest/undertow/SampleUndertowApplication.java
  • Regarding jetty, I've used it as the model to follow.

  • I've not tackled netty yet, since there are no tests and maybe I need to create them, that's for you to decide (we may need another issue for that?).

  • I've already fixed the Tomcat server.

  • The Undertow server is more complicated. There is no trace of context path in the UndertowWebServer class, and the context can't be retrieved on build phase by design, only provided via a Handler. As far as I understand, it can only be retrieved when handling requests. Also, the only trace of context path is in UndertowServletWebServer (and associated factory), where they do use the Handler to provide the context path. So... providing the context path to the UndertowWebServer would need modification of several classes related to the creation of the UndertowWebServer, which could imply breaking changes/me breaking something (Not sure there are proper tests for that).

  • Finally, should I create the PR from my branch?

@philwebb
Copy link
Member

Hi @vicaba, thanks for the update.

I've not tackled netty yet, since there are no tests and maybe I need to create them, that's for you to decide (we may need another issue for that?).

We can leave Netty for now. One of the team can always pick that up when we look at merging the PR.

Finally, should I create the PR from my branch?

Yes please. We probably won't get around to reviewing it until the new year as we've not yet branched for 3.3 development.

@wilkinsona
Copy link
Member Author

I've not tackled netty yet, since there are no tests

FWIW, we do have tests for Netty, including smoke tests. spring-boot-smoke-test-webflux is one.

@vicaba
Copy link

vicaba commented Dec 14, 2023

Hi @philwebb, @wilkinsona, thank you for the info, I will take a look at the Netty situation this weekend and open the PR.

@wilkinsona wilkinsona changed the title Log WebServer's context path consistently Make WebServer's started log messages more consistent Jan 10, 2024
@wilkinsona wilkinsona modified the milestones: 3.x, 3.3.x Jan 10, 2024
@wilkinsona wilkinsona assigned wilkinsona and unassigned vicaba Jan 10, 2024
@wilkinsona
Copy link
Member Author

wilkinsona commented Jan 10, 2024

While we're doing this, we can also improve the logging of the protocol (http or https) across the four different web servers.

@wilkinsona wilkinsona changed the title Make WebServer's started log messages more consistent Make WebServers' started log messages more consistent Jan 10, 2024
@wilkinsona wilkinsona modified the milestones: 3.3.x, 3.3.0-M1 Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants