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

fix: Log4j configurations and wording #11395

Merged
merged 6 commits into from
Nov 7, 2024
Merged

Conversation

vy
Copy link
Contributor

@vy vy commented Sep 19, 2024

The current Log4j 2 documentation contains configurations with deprecated and redundant packages attributes, which cause runtime warnings for users – see apache/logging-log4j2#2966. This PR

  • Removes the usage of the deprecated (and redundant) packages attribute
  • Removes the usage of the redundant status attribute
  • Adds XML namespace (to enable IDE assistance while users are editing)
  • Adapt Log4j component and XML naming conventions
  • Replaces occurrences of Log4j2 in text with Log4j 2

IS YOUR CHANGE URGENT?

Not urgent, can wait up to 1 week+

REVIEWERS

PRE-MERGE CHECKLIST

Make sure you've checked the following before merging your changes:

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs (subject matter experts)
  • PR was reviewed and approved by a member of the Sentry docs team

Copy link

vercel bot commented Sep 19, 2024

@vy is attempting to deploy a commit to the Sentry Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Sep 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
changelog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2024 9:05am
develop-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2024 9:05am
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2024 9:05am

@lizokm
Copy link
Contributor

lizokm commented Sep 19, 2024

@bitsandfoxes could you take a look at this when you have a moment?

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

@vy, looks great, but I would skip the schema location in the examples. Since Sentry is not in the generated XML Schema, the examples would not validate.

Some context for the Sentry team

We recently added a log4j-docgen tool that generates documentation and XML schemas from the Javadoc of elements annotated with @Plugin*.
You can find more information on the Log4j Docgen website.

A priori the tool could generate an XML Schema, which includes the Sentry element and all its options, but it is still pretty much rough around the edges and currently we only use it for our internal artifacts.

The long term plan, however, is:

  • We ask Log4j Plugin vendors to publish a *-log4jdoc.xml artifact with each release.
  • We configure Dependabot to watch for updates to sentry-log4j2.
  • At each new release we regenerate our Plugin Reference to include Sentry,
  • At each new release we regenerate our XSD Schemas to include a Sentry element.

As I mentioned before, right now such a process would be far from being easy, but in time it could show the size of the Log4j community and make logging.apache.org more of a common effort.

docs/platforms/java/common/legacy/log4j2/index.mdx Outdated Show resolved Hide resolved
platform-includes/getting-started-config/java.log4j2.mdx Outdated Show resolved Hide resolved
@vy
Copy link
Contributor Author

vy commented Sep 19, 2024

@ppkarwasz, Thanks so much for the review.

I would skip the schema location in the examples. Since Sentry is not in the generated XML Schema, the examples would not validate.

I prefer to keep the schema location, since, contrary to the current situation (and your proposal) where there is no IDE assistance, my proposal will enable complete IDE assistance except for one line: <Sentry .../>. As can be seen from the user report in apache/logging-log4j2#2966, a typical Sentry user's log4j2.xml contains dozens of lines of code that can take advantage of the XML schema. All in all, I leave it at the discretion of the Sentry team whether to keep the XML schema location or not.

The long term plan, however, is:

Log4j team can ping Sentry again when this plan is realized.

@ppkarwasz
Copy link
Contributor

I prefer to keep the schema location, since, contrary to the current situation (and your proposal) where there is no IDE assistance, my proposal will enable complete IDE assistance except for one line: <Sentry .../>. As can be seen from the user report in apache/logging-log4j2#2966, a typical Sentry user's log4j2.xml contains dozens of lines of code that can take advantage of the XML schema. All in all, I leave it at the discretion of the Sentry team whether to keep the XML schema location or not.

I think this is a problem that <xsd:include> could fix. A small XML schema could include the one at logging.apache.org add a Sentry type and override the org.apache.logging.log4j.core.Appender element group.

@lizokm
Copy link
Contributor

lizokm commented Sep 26, 2024

@bitsandfoxes Hi! Could you take a look at this PR when you have a moment?

@bitsandfoxes
Copy link
Contributor

@adinauer this looks like it's somewhere in your area of the world.

<Loggers>
<Root level="INFO">
<AppenderRef ref="CONSOLE"/>
<AppenderRef ref="SENTRY" level="ERROR"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason for adding the level here, compared to the original?
Do you want to highlight the possibility of overriding the level like in docs/platforms/java/common/legacy/log4j2/index.mdx? If so, I would suggest to also add a comment here, like in the mentioned file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lbloder, earlier the snippet was as follows:

<Root level="info">
  <AppenderRef ref="Sentry"/>
  <AppenderRef ref="Console"/>
</Root>

Though this didn't capture the configuration stated at the beginning of the java.log4j2.mdx file:

The following example using the log4j2.xml format to configure a ConsoleAppender that logs to standard out at the INFO level, and a SentryAppender that logs to the Sentry server at the ERROR level.

That is, sending ERRORs (and of higher severity) to Sentry was misconfigured – I fixed it.

Do you want to highlight the possibility of overriding the level like in docs/platforms/java/common/legacy/log4j2/index.mdx...

I am not able to follow you. I fixed legacy/log4j2/index.mdx too, since that document was stating:

The following example configures a ConsoleAppender that logs to standard out at the INFO level and a SentryAppender that logs to the Sentry server at the WARN level.

Hence, AFAIC, all configurations now match what is stated to be expected from them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vy Sorry for the confusion.
Thank you, for fixing this, I just looked at the differences in the xml and thus overlooked the fact that the description was wrong in the first place.

I am not able to follow you. I fixed legacy/log4j2/index.mdx too, since that document was stating
I was referring to the inline comment above the AppenderRef that might be helpful for users copying the xml.
For example:

<Root level="info">
    <AppenderRef ref="CONSOLE"/>
		<!-- Note that the Sentry logging threshold is overridden to the ERROR level -->
    <AppenderRef ref="SENTRY" level="ERROR"/>
</Root>

@lbloder
Copy link
Contributor

lbloder commented Oct 8, 2024

@vy @ppkarwasz I had a quick sync internally, and for now, we would like to keep the schema out of the examples due to the xml not being valid against the schema.

We will look into @ppkarwasz suggestion on creating our own schema that includes the Sentry element.

Also, whenever your long-term plan is realized, please let us know.

@vy
Copy link
Contributor Author

vy commented Oct 8, 2024

@lbloder, how do you plan to proceed with this PR?

@lbloder
Copy link
Contributor

lbloder commented Oct 8, 2024

@vy
Would you be so kind and apply the changes suggested by @ppkarwasz?

Then I think we can move foward and approve.

Thanks again for doing this 👍

vy and others added 3 commits October 8, 2024 13:31
Co-authored-by: Piotr P. Karwasz <piotr@github.copernik.eu>
Co-authored-by: Piotr P. Karwasz <piotr@github.copernik.eu>
Co-authored-by: Piotr P. Karwasz <piotr@github.copernik.eu>
@vy
Copy link
Contributor Author

vy commented Oct 8, 2024

@vy Would you be so kind and apply the changes suggested by @ppkarwasz?

Done.

@lizokm
Copy link
Contributor

lizokm commented Oct 10, 2024

@adinauer would you take a look when you have a moment please?

@getsantry
Copy link
Contributor

getsantry bot commented Nov 1, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Nov 1, 2024
@vy
Copy link
Contributor Author

vy commented Nov 2, 2024

@lbloder, @adinauer, would you mind reviewing this PR one last time, please? It is ready to be merged.

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Due to IntelliJ still complaining, we're removing the xmlns.

platform-includes/getting-started-config/java.log4j2.mdx Outdated Show resolved Hide resolved
docs/platforms/java/common/legacy/log4j2/index.mdx Outdated Show resolved Hide resolved
@lbloder
Copy link
Contributor

lbloder commented Nov 5, 2024

@vy Thank your for bearing with us.
After testing with IntelliJ it would still mark the Sentry tag as not allowed, so we had to remove the xmlns attribute from the namespace. We will have to introduce this along with the schema once the [Log4j Docgen](https://logging.apache.org/log4j/tools/log4j-docgen.html) is ready.

We already pushed this change and approved the PR.

@vy
Copy link
Contributor Author

vy commented Nov 5, 2024

@lbloder, @bitsandfoxes still appears as the last pending reviewer. Is there anything else I can do to get this PR merged?

Copy link
Contributor

@lbloder lbloder left a comment

Choose a reason for hiding this comment

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

Sorry @vy, forgot to approve

@vy
Copy link
Contributor Author

vy commented Nov 5, 2024

@lbloder, thanks for the prompt reaction. Yet AFAICS, your approval is not sufficient:
image

@adinauer
Copy link
Member

adinauer commented Nov 6, 2024

Sorry for the delays and hickups, trying to figure out what's going on with CI so we can get this PR merged.

@adinauer adinauer merged commit 74099cc into getsentry:master Nov 7, 2024
10 of 11 checks passed
martinhaintz pushed a commit that referenced this pull request Nov 11, 2024
* master: (39 commits)
  getsentry/relay@403f2bf
  feat(native): add backend trade-offs to Advanced Usage (#11721)
  🔧 chore(api): add additional troubleshooting instructions to publish api docs (#11772)
  Bump API schema to 02adb960 (#11773)
  Update metric-alert-config.mdx (#11760)
  update w-9 tax form (#11764)
  chore: silence fetch cache error due to size limit (#11767)
  fix Ureal engine DSN placeholder (#11769)
  feat: Banner pathname regex (#11761)
  docs(nuxt): Add troubleshoot pnpm (#11720)
  Docker Desktop to Colima fix (#11752)
  Bump API schema to d51784a6 (#11759)
  ref(feedback): update user feedback docs to include external issue linking & auto issue creation (#11729)
  getsentry/relay@d23c39a
  Changed /store to /envelope (because /store does not exist anymore (#11755)
  Refactor arcade embeds (#11722)
  feat(js): Add `skipBrowserExtensionCheck` documentation (#11693)
  getsentry/relay@a7d9096
  fix mermaid charts in dark mode (#11754)
  fix: Log4j configurations and wording (#11395)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants