Add standardized property to distinguish a group of applications #39957
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
wilkinsona
changed the title
|
Thanks for the PR, @wanger26. You've marked it as a draft but it looks pretty complete to me. Did you have some other changes in mind that you wanted to make before it's ready for review? |
wilkinsona
added
the
status: waiting-for-feedback
|
Sorry for the delay on this PR @wilkinsona, I have completed all the changes I was hoping to make but was holding off on publishing the PR until I got a green build. I was hoping to pick this up at some point this week and get the build green. |
spring-projects-issues
added
status: feedback-provided
- Add a standardized property to provide some indicator that a set of applications are part of a larger "business application" so that they can be viewed in metrics, portals, traces and more spring-projects#39913 Signed-off-by: Jakob Wanger <[email protected]>
philwebb
added
the
for: team-meeting
philwebb
added
type: enhancement
| /** | ||
| * Default value for application group if {@code spring.application.group} is not set. | ||
| */ | ||
| private static final String DEFAULT_APPLICATION_GROUP = "unknown_group"; |
There was a problem hiding this comment.
I think default is better than unknown here.
There was a problem hiding this comment.
I disagree. unknown_group aligns with the existing unknown_service that's used when there's no application name.
There was a problem hiding this comment.
unknown_groupaligns with the existingunknown_servicethat's used when there's no application name.
It should defaults to a more meaningful name, for example the default namespace in kubernetes is default not unknown.
Most of applications will specify their own spring.application.name but leave spring.application.group absent, they are not equal in nature, I don't think it make sense to align them.
There was a problem hiding this comment.
I'm afraid I still disagree. This feature isn't specific to Kubernetes so aligning with its default namespace does not make sense. Furthermore, it isn't a default group but an indication that we do not know the application's group. As such, I think unknown_group is more meaningful as it accurately reflects the situation that we're in.
There was a problem hiding this comment.
The service name is supposed to be unique per application. It not being specified means that it is indeed unknown, as there cannot be any sensible default value that actually makes sense. The groups are shared, so it not being specified is a pretty good indicator that it belongs to the (shared) default group, not the shared unknown group. unknown_group makes it seem like there is something wrong with the application configuration, whereas you have just opted to not specify an actual group.
There was a problem hiding this comment.
It not being specified means that it is indeed unknown, as there cannot be any sensible default value that actually makes sense.
That is the case here.
The groups are shared, so it not being specified is a pretty good indicator that it belongs to the (shared) default group
There's no "the groups" here. As described in #39913, a group can be a company, org, or team, or pretty much whatever else makes sense for grouping a set of applications. There's no concept of a default group as we don't know that such a thing makes sense in any given deployment environment.
unknown_groupmakes it seem like there is something wrong with the application configuration, whereas you have just opted to not specify an actual group.
"unknown" doesn't imply that something is wrong, all it means is that the group is unknown. I think that accurately reflects the situation as the group really is not known.
There was a problem hiding this comment.
IMO Tag service.group shouldn't be added if it's unknown_group, It doesn't make sense from observation perspective but increase network packet size.
There was a problem hiding this comment.
I agree. We only set service.name to unknown_application because it's a required attribute in otel. This isn't the case for service.group, so we can just omit it, if it's not set.
| ---- | ||
|
|
||
| NOTE: In the example above, configprop:logging.include-application-name[] is set to `false` to avoid the application name being duplicated in the log messages (configprop:logging.pattern.correlation[] already contains it). | ||
| NOTE: In the example above, configprop:logging.include-application-name[] and configprop:logging.include-application-group[] is set to `false` to avoid the application name being duplicated in the log messages (configprop:logging.pattern.correlation[] already contains it). |
There was a problem hiding this comment.
include-application-group does not need to be set to false here as the preceding example of a custom correlation pattern doesn't include spring.application.group. The example shouldn't be changed as it matches Spring Cloud Sleuth's behavior. I think the change on this line should be reverted instead.
| @@ -6,15 +6,16 @@ Default logback configuration provided for import | |||
|
|
|||
| <included> | |||
| <conversionRule conversionWord="applicationName" converterClass="org.springframework.boot.logging.logback.ApplicationNameConverter" /> | |||
| <conversionRule conversionWord="applicationGroup" converterClass="org.springframework.boot.logging.logback.ApplicationGroupConverter" /> | |||
There was a problem hiding this comment.
Minor nit: these rules are ordered based on the conversion word so applicationGroup should go before applicationName.
Change Summary
Issue