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

LOG4J2-2785: Added support for pattern Layout to abbreviate logger or class names except n rightmost words #542

Closed
wants to merge 1 commit into from

Conversation

spannm
Copy link
Contributor

@spannm spannm commented Jul 1, 2021

Hi, I've added a specialized abbreviator named DynamicWordAbbreviator to address the specific requirement of this feature request. Javadoc added and JUnit test updated to cover the new feature.

@spannm spannm closed this Jul 2, 2021
@spannm spannm deleted the LOG4J2-2785 branch July 2, 2021 11:31
@spannm spannm restored the LOG4J2-2785 branch July 2, 2021 11:32
@spannm spannm reopened this Jul 2, 2021
Comment on lines 412 to 416
String[] words = original.split("\\.");
for (int i = 0; i < words.length - rightWordCount; i++) {
words[i] = words[i].substring(0, 1);
}
destination.append(String.join(".", words));
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very expensive. String.split uses regex and allocates a new string for each segment. Substring allocates another string per abbreviated segment, and the final String.join creates a string representation of the data that's added to the StringBuilder. I think allocation can be entirely avoided here by tacking indexes, and writing directly to the Destination buffer.

It could be interesting to run the benchmarks comparing 1.1* to the existing 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback.
Afaik String#split does not create a Pattern object for certain one-character patterns such as here.
I've rewritten the method for improved efficiency.

if (original != null && destination != null) {
String[] words = original.split("\\.");
for (int i = 0; i < words.length - rightWordCount; i++) {
words[i] = words[i].substring(0, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worthwhile to add test coverage for an input string along the lines of org..example.Name with two periods in a row, I think this substring will throw an IndexOutOfBoundsException in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you're right. I've fixed this and improved test coverage.

@carterkozak
Copy link
Contributor

When I have some time to tinker, I think this feature could be added to the existing abbreviator. All existing patterns effectively use rightWordCount=1, however we could make that configurable and allow existing options to work with this.

Something along the lines of

int limit = input.length();
for (int i = 0; I < rightWordCount; I++) {
    limit = input.lastIndexOf('.', limit);
    if (limit < 0) {
        return input;
    }
}
// call into standard abbreviator.

@spannm
Copy link
Contributor Author

spannm commented Aug 27, 2021

[..] All existing patterns effectively use rightWordCount=1, however we could make that configurable and allow existing options to work with this.

@carterkozak
I don't think this is the case. Consider patterns such as 1.1.1.*. The number of unmodified words at the right side depends on the length of the string to be abbreviated, not on the pattern.
NameAbbreviator is abstract. Implementations are MaxElementAbbreviator and PatternAbbreviator.
I don't see a good way of covering the feature request within these two implementations, and have therefore added DynamicWordAbbreviator.

@spannm
Copy link
Contributor Author

spannm commented Sep 8, 2021

@carterkozak Here's another small update to my pull request.
Can you merge it into the release branch please?

@vy vy force-pushed the release-2.x branch 2 times, most recently from b7d26d0 to 0a86ecb Compare October 12, 2021 22:31
@spannm
Copy link
Contributor Author

spannm commented Feb 2, 2022

"we love pull requests" they say
you send them prs
you listen to their criticism
you incorporate their feedback
you think you're done
then nothing happens
forever
wtf

@carterkozak
Copy link
Contributor

@sman-81 we are all volunteers and have limited time to devote to review. Your most recent comment comes across as entitled and rude. Please work on that. In your position, I might have replied on the PR with "Is there anything I can do to get this into a mergable state?"

you listen to their criticism
you incorporate their feedback

I asked you twice to avoid string.split in favor of indexOf/lastIndexOf, but it's still used.

@spannm
Copy link
Contributor Author

spannm commented Feb 3, 2022

@carterkozak

I'm a volunteer just like you.
My pull request is from July 1st, thus seven months old by now!
The last comment from anyone other than me was August 12, 2021.
Such discourages people from contributing to the project.

I asked you twice to avoid string.split in favor of indexOf/lastIndexOf, but it's still used.

And you are wrong. String#split does not create a Pattern object for certain one-character patterns such as here, as I have pointed out to you (see comment on August 5, 2021).

@carterkozak
Copy link
Contributor

The point is that string.split is inefficient, regardless of internal details because it's allocating several new strings unnecessarily.

@spannm
Copy link
Contributor Author

spannm commented Feb 4, 2022

Hi there @carterkozak now String#split is gone. I have also squashed commits into one. Please take a look.

@spannm spannm closed this Aug 29, 2022
@ppkarwasz ppkarwasz reopened this Dec 21, 2022
@rgoers
Copy link
Member

rgoers commented Dec 30, 2022

This patch was manually extracted and merged since the backing repo has been deleted.

@rgoers rgoers closed this Dec 30, 2022
@spannm
Copy link
Contributor Author

spannm commented Jan 3, 2023

Hard to believe my PR actually got merged after a little over 1,5 years :)

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.

None yet

4 participants