Skip to content

Conversation

htztomic
Copy link
Contributor

This is a fix to GitHub issue #16680 . The LoggerGroupsEndpoint gives support for GET configurations of logger groups or a single logger group as well configuring the LogLevel of a specific logger group.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 15, 2019
@mbhave mbhave added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 15, 2019
@mbhave mbhave added this to the 2.2.x milestone Jul 15, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I had a quick look at this enhancement a while back and was wondering if we couldn't use the existing loggers endpoint for that. As you went the route of a dedicated endpoint, I'd love to hear your thoughts.

I've also added a few questions/suggestions.


@Configuration(proxyBeanMethods = false)
@ConditionalOnAvailableEndpoint(endpoint = LoggerGroupsEndpoint.class)
public class LoggersGroupEndpointAutoConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

Without looking at the concrete details, I would have updated the existing LoggersEndpoint to handle both logger names and groups. While we're at the mercy of having a group having the same name as a logger, I wonder if handling them in a single place wouldn't be easier.

}

@Test
void runWithNoLoggerGroupsShouldNotHaveEndpointBean() {
Copy link
Member

Choose a reason for hiding this comment

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

I am not entirely sure I get the rationale of this. IMO, If we go down the route of having a dedicated endpoint, I don't think it should 404 depending of the fact that at least a group is defined or not.

* @author HaiTao Zhang
* @since 2.2.0
*/
public class LoggerGroups {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered updating LoggingSystem rather than registering another singleton in the context? Given that became a feature handled there, perhaps we should expose it rather than having to lookup another type.

@snicoll
Copy link
Member

snicoll commented Jul 17, 2019

@htztomic I missed the conversation of using a separate endpoint while I was on PTO, sorry about that. Please ignore that part of the review for now.

@htztomic htztomic force-pushed the gh-16680 branch 2 times, most recently from 33b0fda to c829a89 Compare July 19, 2019 01:01
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jul 19, 2019
@htztomic htztomic force-pushed the gh-16680 branch 2 times, most recently from 13dedf6 to bbaf119 Compare July 20, 2019 01:27
@wilkinsona
Copy link
Member

When we added support for logging groups we did so without making any, I think, changes to the logging systems. I wonder if we should strive to do the same here? Would it work if we pulled out a separate class, say LoggerGroups, that was responsible for managing the groups and passing the level changes for each logger in a group down to the LoggingSystem? A LoggerGroups instance could be made available as a bean in a similar manner to how the LoggingSystem is made available.

@snicoll
Copy link
Member

snicoll commented Jul 20, 2019

Gah, that's exactly what @htztomic had and I suggested in my review to avoid registering yet another bean.

@wilkinsona
Copy link
Member

IMO, registering another bean is preferable to adding another responsibility to LoggingSystem implementations. A middle-ground would be to introduce LoggerGroups as @htztomic originally proposed, but provide access to it via a getLoggerGroups() on LoggingSystem rather than registering it as a bean. I still prefer the bean registration approach, however. There's no technical need for LoggingSystem to know about logger groups and I'd prefer to keep that separation.

@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jul 24, 2019
@mbhave mbhave self-assigned this Jul 24, 2019
mbhave added a commit that referenced this pull request Jul 30, 2019
@mbhave mbhave closed this in 6e1fb5a Jul 30, 2019
@mbhave mbhave changed the title LoggerGroupsEndpoint created for modification of logger groups. Add support for configuring logger groups via loggers endpoint Jul 30, 2019
@mbhave mbhave modified the milestones: 2.2.x, 2.2.0.M5 Jul 30, 2019
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 this pull request may close these issues.

5 participants