sentry-docs icon indicating copy to clipboard operation
sentry-docs copied to clipboard

fix: Log4j configurations and wording

Open vy opened this issue 1 year ago • 12 comments

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

  • @maciejwalkowiak
  • @adinauer
  • @ppkarwasz

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

vy avatar Sep 19 '24 11:09 vy

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

A member of the Team first needs to authorize it.

vercel[bot] avatar Sep 19 '24 11:09 vercel[bot]

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

vercel[bot] avatar Sep 19 '24 17:09 vercel[bot]

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

lizokm avatar Sep 19 '24 17:09 lizokm

@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.

vy avatar Sep 19 '24 18:09 vy

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.

ppkarwasz avatar Sep 19 '24 20:09 ppkarwasz

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

lizokm avatar Sep 26 '24 22:09 lizokm

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

bitsandfoxes avatar Oct 07 '24 09:10 bitsandfoxes

@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.

lbloder avatar Oct 08 '24 10:10 lbloder

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

vy avatar Oct 08 '24 10:10 vy

@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 👍

lbloder avatar Oct 08 '24 11:10 lbloder

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

Done.

vy avatar Oct 08 '24 11:10 vy

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

lizokm avatar Oct 10 '24 18:10 lizokm

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[bot] avatar Nov 01 '24 07:11 getsantry[bot]

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

vy avatar Nov 02 '24 18:11 vy

@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.

lbloder avatar Nov 05 '24 15:11 lbloder

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

vy avatar Nov 05 '24 19:11 vy

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

vy avatar Nov 05 '24 19:11 vy

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

adinauer avatar Nov 06 '24 08:11 adinauer