Skip to content

Commit

Permalink
Fixed a bug where logs produced by Flags were not appended to `Requ…
Browse files Browse the repository at this point in the history
…estContextExportingAppender` (line#5361)

Motivation:

If `Flags` class is initialized before `RequestContextExportingAppender` is registered to the root logger,
the logs produced `Flags` in the initialization of the class are silently ignored.
See line#5327 for the details.

Modifications:

- Lazily create `RequestContextExporterBuilder` to not initialize `Flags` while ` RequestContextExportingAppender` is being initialized.
  - `RequestContextExporterBuilder` is created when `FlagsLoaded.get()` is true.

Result:

- `Flags` now correctly logs all messages when `RequestContextExportingAppender` is configured.
- Fixes line#5327
  • Loading branch information
ikhoon authored Jan 22, 2024
1 parent b0d3044 commit 7031340
Showing 1 changed file with 30 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.function.Consumer;
import java.util.function.Function;

import org.slf4j.MDC;
Expand Down Expand Up @@ -67,10 +68,10 @@ public final class RequestContextExportingAppender
private static final Splitter KEY_SPLITTER = Splitter.on(',').trimResults();

private final AppenderAttachableImpl<ILoggingEvent> aai = new AppenderAttachableImpl<>();
private final RequestContextExporterBuilder builder = RequestContextExporter.builder();
@Nullable
private RequestContextExporter exporter;
private boolean needsHashMap;
private Consumer<RequestContextExporterBuilder> customizer = unused -> {};

@VisibleForTesting
RequestContextExporter exporter() {
Expand All @@ -83,7 +84,7 @@ RequestContextExporter exporter() {
*/
public void addBuiltIn(BuiltInProperty property) {
ensureNotStarted();
builder.builtIn(property);
configureExporter(builder -> builder.builtIn(property));
}

/**
Expand All @@ -94,7 +95,7 @@ public void addBuiltIn(BuiltInProperty property) {
*/
public void addAttribute(String alias, AttributeKey<?> attrKey) {
ensureNotStarted();
builder.attr(alias, attrKey);
configureExporter(builder -> builder.attr(alias, attrKey));
}

/**
Expand All @@ -109,7 +110,7 @@ public void addAttribute(String alias, AttributeKey<?> attrKey, Function<?, Stri
requireNonNull(alias, "alias");
requireNonNull(attrKey, "attrKey");
requireNonNull(stringifier, "stringifier");
builder.attr(alias, attrKey, stringifier);
configureExporter(builder -> builder.attr(alias, attrKey, stringifier));
}

/**
Expand All @@ -118,7 +119,7 @@ public void addAttribute(String alias, AttributeKey<?> attrKey, Function<?, Stri
public void addRequestHeader(CharSequence name) {
ensureNotStarted();
requireNonNull(name, "name");
builder.requestHeader(name);
configureExporter(builder -> builder.requestHeader(name));
}

/**
Expand All @@ -127,7 +128,7 @@ public void addRequestHeader(CharSequence name) {
public void addResponseHeader(CharSequence name) {
ensureNotStarted();
requireNonNull(name, "name");
builder.responseHeader(name);
configureExporter(builder -> builder.responseHeader(name));
}

/**
Expand All @@ -137,7 +138,7 @@ public void addResponseHeader(CharSequence name) {
public void setPrefix(String prefix) {
requireNonNull(prefix, "prefix");
checkArgument(!prefix.isEmpty(), "prefix must not be empty");
builder.prefix(prefix);
configureExporter(builder -> builder.prefix(prefix));
}

/**
Expand All @@ -148,7 +149,7 @@ public void setPrefix(String prefix) {
public void setExport(String mdcKey) {
requireNonNull(mdcKey, "mdcKey");
checkArgument(!mdcKey.isEmpty(), "mdcKey must not be empty");
builder.keyPattern(mdcKey);
configureExporter(builder -> builder.keyPattern(mdcKey));
}

/**
Expand All @@ -159,11 +160,13 @@ public void setExport(String mdcKey) {
public void setExports(String mdcKeys) {
requireNonNull(mdcKeys, "mdcKeys");
checkArgument(!mdcKeys.isEmpty(), "mdcKeys must not be empty");
KEY_SPLITTER.split(mdcKeys)
.forEach(mdcKey -> {
checkArgument(!mdcKey.isEmpty(), "comma-separated MDC key must not be empty");
builder.keyPattern(mdcKey);
});
configureExporter(builder -> {
KEY_SPLITTER.split(mdcKeys)
.forEach(mdcKey -> {
checkArgument(!mdcKey.isEmpty(), "comma-separated MDC key must not be empty");
builder.keyPattern(mdcKey);
});
});
}

/**
Expand All @@ -172,7 +175,7 @@ public void setExports(String mdcKeys) {
*/
public void setExportGroup(ExportGroupConfig exportGroupConfiguration) {
requireNonNull(exportGroupConfiguration, "exportGroupConfiguration");
builder.exportGroup(exportGroupConfiguration.build());
configureExporter(builder -> builder.exportGroup(exportGroupConfiguration.build()));
}

private void ensureNotStarted() {
Expand All @@ -181,6 +184,15 @@ private void ensureNotStarted() {
}
}

/**
* Stores the specified {@link RequestContextExporterBuilder} customizer to be lazily applied when
* the exporter is built.
*/
private void configureExporter(Consumer<RequestContextExporterBuilder> customizer) {
requireNonNull(customizer, "customizer");
this.customizer = this.customizer.andThen(customizer);
}

@Override
protected void append(ILoggingEvent eventObject) {
if (exporter == null) {
Expand All @@ -192,6 +204,10 @@ protected void append(ILoggingEvent eventObject) {
aai.appendLoopOnAppenders(eventObject);
return;
}
// Build the exporter lazily to prevent the customizer from initializing Flags.
// See: https://github.com/line/armeria/issues/5327
final RequestContextExporterBuilder builder = RequestContextExporter.builder();
customizer.accept(builder);
exporter = builder.build();
}
final Map<String, String> contextMap = exporter.export();
Expand Down

0 comments on commit 7031340

Please sign in to comment.