-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Fix prefix logging #20429
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 prefix logging #20429
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.common.logging; | ||
|
|
||
| import org.apache.logging.log4j.Level; | ||
| import org.apache.logging.log4j.Marker; | ||
| import org.apache.logging.log4j.MarkerManager; | ||
| import org.apache.logging.log4j.message.Message; | ||
| import org.apache.logging.log4j.spi.ExtendedLogger; | ||
| import org.apache.logging.log4j.spi.ExtendedLoggerWrapper; | ||
|
|
||
| import java.lang.ref.WeakReference; | ||
| import java.util.WeakHashMap; | ||
|
|
||
| class PrefixLogger extends ExtendedLoggerWrapper { | ||
|
|
||
| private final String prefix; | ||
| private final Marker marker; | ||
|
|
||
| public String prefix() { | ||
| return prefix; | ||
| } | ||
|
|
||
| private static final WeakHashMap<String, WeakReference<Marker>> markers = new WeakHashMap<>(); | ||
|
|
||
| PrefixLogger(final ExtendedLogger logger, final String name, final String prefix) { | ||
| super(logger, name, null); | ||
| this.prefix = prefix; | ||
|
|
||
| final String actualPrefix = (prefix == null ? "[] " : prefix).intern(); | ||
| final Marker actualMarker; | ||
| synchronized (markers) { | ||
| final WeakReference<Marker> marker = markers.get(actualPrefix); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need the whole weak shabang? I get you are trying to reuse marker objects, and I guess the idea is to drop the map once it's not needed any more. If that's correct, I'm a bit queasy about it - I rather see a simpler concurrent map (is protecting with sync(markers) enough for weak references that the GC may collect?)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Using weak references allows these to be cleaned up if they are in fact no longer in use.
A concurrent map will just be a memory leak, we will never clean up prefixes no longer used (think of index and shard prefixes).
The synchronization is not related to the weak references, it's just to protect the map from concurrent modifications (which are not thread safe).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A sad, but valid point. add a comment?
What happens if Also to clarify - this method can not guarantee markers will not be created twice with the same prefix, but it limits it considerably, correct? Last - I'm still not convinced we shouldn't just go with creating a marker on each request and be done with it (it's on more object per logger, which I don't think is a huge deal).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I pushed 40dae7a.
I pushed 40dae7a.
Why do you think that?
For a cluster with 365 indices, 5 shards per index, your proposal will lead to 37292 unique marker objects; with my proposal there are 2194 marker objects. That's why I think this is worth doing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does having a hard reference to an object guarantees all weak references to it are kept around? I was worried the GC might decide to remove a weak reference just because, in which case we will create another marker. Again - not saying this is wrong, but looking to learn.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes (although it's usually called a "strong reference"). |
||
| if (marker == null || marker.get() == null) { | ||
| actualMarker = new MarkerManager.Log4jMarker(actualPrefix); | ||
| markers.put(actualPrefix, new WeakReference<>(actualMarker)); | ||
| } else { | ||
| actualMarker = marker.get(); | ||
| } | ||
| } | ||
| this.marker = actualMarker; | ||
| } | ||
|
|
||
| @Override | ||
| public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) { | ||
| assert marker == null; | ||
| super.logMessage(fqcn, level, this.marker, message, t); | ||
| } | ||
|
|
||
| } | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a deviation from the 2.x version where a null prefix will be completely dropped. Are you sure this is a good change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log4j will not allow a null marker, but I can replace this with the empty string if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be cleaner? not a biggy though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed 7c16c6c.