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

Remove the EventTemplateAdditionalFields which were upstreamed in log4j2 #334

Open
aabmass opened this issue Apr 23, 2024 · 10 comments
Open
Assignees
Labels
blocked This issue or pull request is waiting on something else, and isn't actionable right now enhancement New feature or request priority: p3

Comments

@aabmass
Copy link
Contributor

aabmass commented Apr 23, 2024

The additional fields are here

<!-- Extend the included GcpLayout to include the trace and span IDs from Mapped
Diagnostic Context (MDC) so that Cloud Logging can correlate Logs and Spans -->
<EventTemplateAdditionalField
key="logging.googleapis.com/trace"
format="JSON"
value='{"$resolver": "mdc", "key": "trace_id"}'
/>
<EventTemplateAdditionalField
key="logging.googleapis.com/spanId"
format="JSON"
value='{"$resolver": "mdc", "key": "span_id"}'
/>
<EventTemplateAdditionalField
key="logging.googleapis.com/trace_sampled"
format="JSON"
value="true"
/>

These were upstreamed into log4j2 in apache/logging-log4j2#2498 but are not yet released. Once it is released, we can bump the version and remove.

@aabmass aabmass added enhancement New feature or request priority: p2 blocked This issue or pull request is waiting on something else, and isn't actionable right now labels Apr 23, 2024
@aabmass aabmass self-assigned this Apr 23, 2024
@aabmass
Copy link
Contributor Author

aabmass commented Jul 15, 2024

Still waiting on log4j2 to release this change.

@aabmass
Copy link
Contributor Author

aabmass commented Oct 3, 2024

This was finally released and we could do it. However the spring boot 2.7.x starter still uses an older log4j2 version, so it requires overriding the version which is a little clunky. Not sure if it's worth moving forward with

main...aabmass:quickstart-log4j-simple

@aabmass
Copy link
Contributor Author

aabmass commented Oct 3, 2024

@psx95 any thoughts on the above?

@psx95
Copy link
Contributor

psx95 commented Oct 3, 2024

I think it's good to replace the one with the updated log4j2 version.

Just a couple suggestions:

  • It would be better to provide the link to the spring boot doc where it provides details on overriding dependency version for transitive dependencies.
  • Clearly mention that log4j2 v2.24 comes bundled with spring-boot vX.x and so specifying this override is not necessary when a higher spring boot version is used.

@aabmass
Copy link
Contributor Author

aabmass commented Oct 3, 2024

  • Clearly mention that log4j2 v2.24 comes bundled with spring-boot vX.x and so specifying this override is not necessary when a higher spring boot version is used.

This might be problematic to discover since I think they backport updates to previous minor versions as needed. Is there an easy way to discover this?

My main concern was whether overriding the log4j2 version would be more complicated/bad practice (i.e. this warning) in spring land vs just copy pasting the log config. I can definitely add a note that "this is not needed for log4j2 2.24+" regardless of decision

@aabmass
Copy link
Contributor Author

aabmass commented Oct 3, 2024

Also, it looks like even the latest spring boot is on too early of a log4j version https://docs.spring.io/spring-boot/appendix/dependency-versions/coordinates.html#:~:text=log4j%2Dcore-,2.23.1,-org.apache.logging

@psx95
Copy link
Contributor

psx95 commented Oct 4, 2024

Also, it looks like even the latest spring boot is on too early of a log4j version https://docs.spring.io/spring-boot/appendix/dependency-versions/coordinates.html#:~:text=log4j%2Dcore-,2.23.1,-org.apache.logging

This does not seem to be an issue - this is log4j-core not the same as log4j2. log4j2 does have a transitive dependency on it, but log4j-core v2.23 is quite recent.

  • Clearly mention that log4j2 v2.24 comes bundled with spring-boot vX.x and so specifying this override is not necessary when a higher spring boot version is used.

This might be problematic to discover since I think they backport updates to previous minor versions as needed. Is there an easy way to discover this?

My main concern was whether overriding the log4j2 version would be more complicated/bad practice (i.e. this warning) in spring land vs just copy pasting the log config. I can definitely add a note that "this is not needed for log4j2 2.24+" regardless of decision

I would personally treat that warning as "try it and if it works you can use it" - but I am not super familiar with Spring projects.

Alternatively, what we can keep the current config as is since it makes more sense for our use-case given that we want to keep supporting this quickstart on Java 11 - and mention it as the reason why we are not moving to the shorter config.
Additionally, in a commented out fashion provide the shorter JsonLayout option that comes bundled with the newer log4j2 version.

Mostly the intent here is to convey to user that the if they can move to Java 17, use the newer spring boot version that supports the updated log4j2 version and the built in GCP JsonLayout.

@aabmass
Copy link
Contributor Author

aabmass commented Oct 4, 2024

This does not seem to be an issue - this is log4j-core not the same as log4j2. log4j2 does have a transitive dependency on it, but log4j-core v2.23 is quite recent.

Mostly the intent here is to convey to user that the if they can move to Java 17, use the newer spring boot version that supports the updated log4j2 version and the built in GCP JsonLayout.

I didn't follow, log4j-core is the logging implementation for log4j2 https://logging.apache.org/log4j/2.x/manual/installation.html#impl-core. From the link above, the latest Spring Boot starter release still does not have the updated GcpLayout.json and would need the workaround. We can sit on this longer, but I doubt the Spring Boot 2 branch will be updated with newer log4j coordinates in any kind of timely fashion.

I would personally treat that warning as "try it and if it works you can use it" - but I am not super familiar with Spring projects.

Yup, idk if I would recommend this to users. But I can certainly add a comment in the XML file. The XML config is still useful even if not using Spring Boot

@psx95
Copy link
Contributor

psx95 commented Oct 4, 2024

Ah, I see so to use the new built-in layout config, the core impl also has to be >= 2.24.

Alternatively, what we can keep the current config as is since it makes more sense for our use-case given that we want to keep supporting this quickstart on Java 11 - and mention it as the reason why we are not moving to the shorter config.
Additionally, in a commented out fashion provide the shorter JsonLayout option that comes bundled with the newer log4j2 version.
Mostly the intent here is to convey to user that the if they can move to Java 17, use the newer spring boot version that supports the updated log4j2 version and the built in GCP JsonLayout.

I think this still holds true regardless, so TL;DR keeping the config same + comments mentioning the use of newer GcpJsonLayout would be good to do.

I would remove the change done in gradle file to avoid confusion. I believe users can figure out overriding from Spring Boot docs.

@aabmass
Copy link
Contributor Author

aabmass commented Oct 4, 2024

SG, opened #379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue or pull request is waiting on something else, and isn't actionable right now enhancement New feature or request priority: p3
Projects
None yet
Development

No branches or pull requests

2 participants