-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Exposing the ability to log deprecated settings at non-critical level #79107
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
Changes from 4 commits
b1dfda3
19788f4
d5884c6
b7b5f75
e25078d
0dd2666
4ac690d
d8c8dde
c90725b
bebb895
841fbb2
d2a82a7
0c18846
9fa7f60
0bb8894
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 |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
|
||
package org.elasticsearch.common.logging; | ||
|
||
import org.apache.logging.log4j.Level; | ||
import org.elasticsearch.Build; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
|
@@ -32,10 +33,11 @@ | |
public class HeaderWarning { | ||
/** | ||
* Regular expression to test if a string matches the RFC7234 specification for warning headers. This pattern assumes that the warn code | ||
* is always 299. Further, this pattern assumes that the warn agent represents a version of Elasticsearch including the build hash. | ||
* is always 299 or 300. Further, this pattern assumes that the warn agent represents a version of Elasticsearch including the build | ||
* hash. | ||
*/ | ||
public static final Pattern WARNING_HEADER_PATTERN = Pattern.compile( | ||
"299 " + // warn code | ||
"(?:299|300) " + // log level code | ||
"Elasticsearch-" + // warn agent | ||
"\\d+\\.\\d+\\.\\d+(?:-(?:alpha|beta|rc)\\d+)?(?:-SNAPSHOT)?-" + // warn agent | ||
"(?:[a-f0-9]{7}(?:[a-f0-9]{33})?|unknown) " + // warn agent | ||
|
@@ -53,15 +55,16 @@ public class HeaderWarning { | |
|
||
/* | ||
* RFC7234 specifies the warning format as warn-code <space> warn-agent <space> "warn-text" [<space> "warn-date"]. Here, warn-code is a | ||
* three-digit number with various standard warn codes specified. The warn code 299 is apt for our purposes as it represents a | ||
* miscellaneous persistent warning (can be presented to a human, or logged, and must not be removed by a cache). The warn-agent is an | ||
* arbitrary token; here we use the Elasticsearch version and build hash. The warn text must be quoted. The warn-date is an optional | ||
* quoted field that can be in a variety of specified date formats; here we use RFC 1123 format. | ||
* three-digit number with various standard warn codes specified, and is left off of this static prefix so that it can be added based | ||
* on the log level received. The warn code will be either 299 or 300 at runtime, which are apt for our purposes as | ||
* they represent miscellaneous persistent warnings (can be presented to a human, or logged, and must not be removed by a cache). | ||
* The warn-agent is an arbitrary token; here we use the Elasticsearch version and build hash. The warn text must be quoted. The | ||
* warn-date is an optional quoted field that can be in a variety of specified date formats; here we use RFC 1123 format. | ||
*/ | ||
private static final String WARNING_PREFIX = | ||
String.format( | ||
Locale.ROOT, | ||
"299 Elasticsearch-%s%s-%s", | ||
" Elasticsearch-%s%s-%s", | ||
Version.CURRENT.toString(), | ||
Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : "", | ||
Build.CURRENT.hash()); | ||
|
@@ -193,10 +196,14 @@ private static boolean assertWarningValue(final String s, final String warningVa | |
* @return a warning value formatted according to RFC 7234 | ||
*/ | ||
public static String formatWarning(final String s) { | ||
return formatWarning(DeprecationLogger.CRITICAL, s); | ||
} | ||
|
||
private static String formatWarning(final Level level, final String s) { | ||
// Assume that the common scenario won't have a string to escape and encode. | ||
int length = WARNING_PREFIX.length() + s.length() + 3; | ||
int length = WARNING_PREFIX.length() + s.length() + 6; | ||
final StringBuilder sb = new StringBuilder(length); | ||
sb.append(WARNING_PREFIX).append(" \"").append(escapeAndEncode(s)).append("\""); | ||
sb.append(level.intLevel() + WARNING_PREFIX).append(" \"").append(escapeAndEncode(s)).append("\""); | ||
return sb.toString(); | ||
} | ||
|
||
|
@@ -311,15 +318,24 @@ public static String getXOpaqueId() { | |
} | ||
|
||
public static void addWarning(String message, Object... params) { | ||
addWarning(THREAD_CONTEXT, message, params); | ||
addWarning(THREAD_CONTEXT, DeprecationLogger.CRITICAL, message, params); | ||
} | ||
|
||
public static void addWarning(Level level, String message, Object... params) { | ||
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. Rather than adding another public method, can you please update the existing uses? It looks like there are ~12 calls to the existing method, and most are in tests, it should be easy to update with eg IntelliJ refactoring tools to add a parameter. 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. Yeah I went back and forth on this. I actually did this and backed it out in an attempt to make this PR smaller. I'll consolidate it into one method. |
||
addWarning(THREAD_CONTEXT, level, message, params); | ||
} | ||
|
||
// package scope for testing | ||
static void addWarning(Set<ThreadContext> threadContexts, String message, Object... params) { | ||
addWarning(threadContexts, DeprecationLogger.CRITICAL, message, params); | ||
} | ||
|
||
// package scope for testing | ||
static void addWarning(Set<ThreadContext> threadContexts, Level level, String message, Object... params) { | ||
final Iterator<ThreadContext> iterator = threadContexts.iterator(); | ||
if (iterator.hasNext()) { | ||
final String formattedMessage = LoggerMessageFormat.format(message, params); | ||
final String warningHeaderValue = formatWarning(formattedMessage); | ||
final String warningHeaderValue = formatWarning(level, formattedMessage); | ||
assert WARNING_HEADER_PATTERN.matcher(warningHeaderValue).matches(); | ||
assert extractWarningValueFromWarningHeader(warningHeaderValue, false) | ||
.equals(escapeAndEncode(formattedMessage)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,10 +99,15 @@ public enum Property { | |
Final, | ||
|
||
/** | ||
* mark this setting as deprecated | ||
* mark this setting as deprecated (critical level) | ||
*/ | ||
Deprecated, | ||
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. It would be nice, as a followup (since it will touch so many files), to rename this. |
||
|
||
/** | ||
* mark this setting as deprecated (warning level) | ||
*/ | ||
DeprecatedWarning, | ||
|
||
/** | ||
* Node scope | ||
*/ | ||
|
@@ -371,7 +376,11 @@ public boolean hasIndexScope() { | |
* Returns <code>true</code> if this setting is deprecated, otherwise <code>false</code> | ||
*/ | ||
public boolean isDeprecated() { | ||
return properties.contains(Property.Deprecated); | ||
return properties.contains(Property.Deprecated) || properties.contains(Property.DeprecatedWarning); | ||
} | ||
|
||
private boolean isDeprecatedWarningOnly() { | ||
return properties.contains(Property.DeprecatedWarning) && properties.contains(Property.Deprecated) == false; | ||
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. We should assert somewhere that both properties do not exist, they should be mutually exclusive. Then this method isn't needed, just check for contains the warning version? 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 added this to the Setting constructor, and updated this method.. |
||
} | ||
|
||
/** | ||
|
@@ -552,10 +561,13 @@ void checkDeprecation(Settings settings) { | |
if (this.isDeprecated() && this.exists(settings)) { | ||
// It would be convenient to show its replacement key, but replacement is often not so simple | ||
final String key = getKey(); | ||
Settings.DeprecationLoggerHolder.deprecationLogger | ||
.critical(DeprecationCategory.SETTINGS, key, | ||
"[{}] setting was deprecated in Elasticsearch and will be removed in a future release! " | ||
+ "See the breaking changes documentation for the next major version.", key); | ||
String message = "[{}] setting was deprecated in Elasticsearch and will be removed in a future release! " | ||
+ "See the breaking changes documentation for the next major version."; | ||
if (this.isDeprecatedWarningOnly()) { | ||
Settings.DeprecationLoggerHolder.deprecationLogger.warn(DeprecationCategory.SETTINGS, key, message, key); | ||
} else { | ||
Settings.DeprecationLoggerHolder.deprecationLogger.critical(DeprecationCategory.SETTINGS, key, message, key); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,11 +35,13 @@ | |
import org.apache.lucene.util.LuceneTestCase.SuppressCodecs; | ||
import org.apache.lucene.util.TestRuleMarkFailure; | ||
import org.apache.lucene.util.TimeUnits; | ||
import org.elasticsearch.Build; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.bootstrap.BootstrapForTesting; | ||
import org.elasticsearch.client.Requests; | ||
import org.elasticsearch.cluster.ClusterModule; | ||
import org.elasticsearch.cluster.metadata.IndexMetadata; | ||
import org.elasticsearch.common.logging.DeprecationLogger; | ||
import org.elasticsearch.core.CheckedRunnable; | ||
import org.elasticsearch.core.RestApiVersion; | ||
import org.elasticsearch.core.SuppressForbidden; | ||
|
@@ -458,11 +460,35 @@ protected final void assertSettingDeprecationsAndWarnings(final String[] setting | |
.toArray(String[]::new)); | ||
} | ||
|
||
protected final void assertSettingDeprecationsAndWarningsIncludingLevel(final Setting<?>[] settings, final String... warnings) { | ||
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. There are only 2 uses of the method above which takes string setting names. Let's collapse all 3 of these methods into one, this is unwieldy having 3 methods of the same name, with no clear distinction of when to call each. 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. This is another one I'd gone back and forth on. I've merged them all into one method now. |
||
assertWarnings( | ||
true, | ||
true, | ||
Stream.concat( | ||
Arrays | ||
.stream(settings) | ||
.map(setting -> String.format( | ||
Locale.ROOT, "%s Elasticsearch-%s%s-%s \"[%s] setting was deprecated in Elasticsearch and will be " + | ||
"removed in a future release! See the breaking changes documentation for the next major version.\"", | ||
setting.getProperties().contains(Setting.Property.Deprecated) ? DeprecationLogger.CRITICAL.intLevel() : | ||
Level.WARN.intLevel(), | ||
Version.CURRENT.toString(), | ||
Build.CURRENT.isSnapshot() ? "-SNAPSHOT" : "", | ||
Build.CURRENT.hash(), | ||
setting.getKey())), | ||
Arrays.stream(warnings)) | ||
.toArray(String[]::new)); | ||
} | ||
|
||
protected final void assertWarnings(String... expectedWarnings) { | ||
assertWarnings(true, expectedWarnings); | ||
} | ||
|
||
protected final void assertWarnings(boolean stripXContentPosition, String... expectedWarnings) { | ||
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. There are 4 uses of this method. Let's update those to call the updated signature rather than adding yet another variant. 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. Done |
||
assertWarnings(stripXContentPosition, false, expectedWarnings); | ||
} | ||
|
||
protected final void assertWarnings(boolean stripXContentPosition, boolean includeLevelCheck, String... expectedWarnings) { | ||
if (enableWarningsCheck() == false) { | ||
throw new IllegalStateException("unable to check warning headers if the test is not set to do so"); | ||
} | ||
|
@@ -473,7 +499,9 @@ protected final void assertWarnings(boolean stripXContentPosition, String... exp | |
} else { | ||
assertNotNull("no warnings, expected: " + Arrays.asList(expectedWarnings), actualWarnings); | ||
final Set<String> actualWarningValues = | ||
actualWarnings.stream().map(s -> HeaderWarning.extractWarningValueFromWarningHeader(s, stripXContentPosition)) | ||
actualWarnings.stream() | ||
.map(s -> includeLevelCheck ? HeaderWarning.escapeAndEncode(s) : | ||
HeaderWarning.extractWarningValueFromWarningHeader(s, stripXContentPosition)) | ||
.collect(Collectors.toSet()); | ||
for (String msg : expectedWarnings) { | ||
assertThat(actualWarningValues, hasItem(HeaderWarning.escapeAndEncode(msg))); | ||
|
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.
Another method seems unnecessary, the only non-test use is in this class, let's just update the few tests using this to pass the level?