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

RequestContextExportingAppender has a dependency on Flags which partially breaks logging #5327

Closed
KarboniteKream opened this issue Dec 5, 2023 · 5 comments · Fixed by #5361
Labels
Milestone

Comments

@KarboniteKream
Copy link
Member

When Armeria is configured with Logback and RequestContextExportingAppender with custom headers, and when multiple Netty versions are present in the classpath, logs for Flags will be missing.

When the following appender with a custom header is configured in Logback:

<appender name="ARMERIA_KAFKA" class="com.linecorp.armeria.common.logback.RequestContextExportingAppender">
  <appender-ref ref="KAFKA" />
  <export>req.headers.x-real-ip</export>
</appender>

The Logback setup flow will behave as follows:

  1. setExport() is called for req.headers.x-real-ip
  2. Eventually, we call ExportGroupBuilder.requestHeader()
  3. Following toHeaderName(), we reach this call to Flags.validateHeaders()
  4. This initializes the Flags class, which initializes TransportType, which initializes TransportTypeProvider
  5. Flags will log the values of all flags, and TransportTypeProvider will validate Netty versions and log a warning in case of inconsistencies

However, since we're still setting up Logback, no appenders are configured on ROOT level, so this information will never be logged.

By removing <export>req.headers.x-real-ip</export> or using req.headers.user-agent (a known and pre-cached header), the call to Flags.validateHeaders() never happens, and the Flags class is initialized after Logback setup is done. In that case, the logs will appear as expected.

We can probably fix this by breaking the dependency between RequestContextExportingAppender and `Flags.

@trustin trustin added the defect label Dec 5, 2023
@trustin trustin added this to the 1.26.4 milestone Dec 5, 2023
@jrhee17
Copy link
Contributor

jrhee17 commented Dec 5, 2023

I haven't been able to analyze the issue in detail, but perhaps we can consider a lazy loading mechanism for each flag value. We have already had multiple issues stemming from cyclic dependency when the Flags class is loaded, so I feel like we may want to take a more general approach

boolean VERBOSE_SOCKET_EXCEPTIONS

to

Supplier<Boolean> VERBOSE_SOCKET_EXCEPTIONS

The downside would be we won't be able to print flag values on initialization. Perhaps we can display this information somewhere else (i.e. DocService) instead.

@trustin trustin modified the milestones: 1.26.4, 1.27.0 Dec 6, 2023
@ikhoon
Copy link
Contributor

ikhoon commented Dec 20, 2023

I tried to make a simple reproducer but failed. It was tricky to align the initialization of the order of Flags and RequestContextExportingAppender. @KarboniteKream Could you provide an example for the problem?

I guess we can keep logging events in RequestContextExportingAppender if no appenders are configured. Afterward, the events may be emitted as an appended is added or start() is called.

@KarboniteKream
Copy link
Member Author

KarboniteKream commented Dec 20, 2023

I've created a branch for armeria-examples/spring-boot-webflux with logback-spring.xml that reproduces the issue:
KarboniteKream/armeria-examples@18c834c

If you run it as is, you'll only see 3 logs from Flags:

com.linecorp.armeria.common.Flags        : useOpenSsl: true (default)
com.linecorp.armeria.common.Flags        : Using OpenSSL: BoringSSL, 0x1010107f
com.linecorp.armeria.common.Flags        : dumpOpenSslInfo: false (default)

If the line <export>req.headers.fake-header</export> is removed or replaced with a well-known header, you'll see all logs for Flags (~70 lines):

com.linecorp.armeria.common.Flags        : verboseExceptionSampler: ExceptionSampler{rate-limit=10} (default)
com.linecorp.armeria.common.Flags        : verboseExceptions: rate-limit=10 (default)
com.linecorp.armeria.common.Flags        : preferredIpV4Addresses: * (default)
com.linecorp.armeria.common.Flags        : verboseSocketExceptions: false (default)
...

@ikhoon
Copy link
Contributor

ikhoon commented Dec 27, 2023

Many thanks for the reproducer. 🙇‍♂️

@ikhoon
Copy link
Contributor

ikhoon commented Dec 27, 2023

The problem was produced with Spring WebFlux module. But Spring MVC didn't have the same issue.
The problem was reproduced by starting main method but failed to reproduce with SpringBootTest
Let me send a patch for that soon.

ikhoon added a commit to ikhoon/armeria that referenced this issue Dec 27, 2023
…estContextExportingAppender`

Motivation:

If `Flags` class is initialized before `RequestContextExportingAppender`
is registered to the root logger,
the logs produced `Flags` during the initialization phase are siliently
ignored.
See line#5327 for the detail.

Modifications:

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

Result:

- `Flags` now correcly logs all message when `RequestContextExportingAppender` is configured.
- Fixes line#5327
minwoox pushed a commit that referenced this issue Jan 22, 2024
…estContextExportingAppender` (#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 #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 #5327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants