Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the OkHttp LOGGER-mode HTTP traffic logging in the MarkLogic Java client so that network log lines are emitted via a stable, dedicated SLF4J logger category (com.marklogic.client.okhttp.network) at INFO level, avoiding the need to enable DEBUG logging on internal impl.* packages.
Changes:
- Route LOGGER-mode output to
com.marklogic.client.okhttp.networkand log atINFOlevel inRedactingHttpLogger. - Add unit tests validating the logger category/level and ensuring internal
impl.*logger levels don’t gate network logs. - Update
OkHttpServicesJavadoc to document the three output targets and the new LOGGER-mode category/level.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| marklogic-client-api/src/main/java/com/marklogic/client/impl/okhttp/RedactingHttpLogger.java | Switch LOGGER-mode logging to a stable named category at INFO and document it in Javadoc. |
| marklogic-client-api/src/test/java/com/marklogic/client/impl/okhttp/RedactingHttpLoggerTest.java | Add logback-based tests to assert LOGGER-mode category/level behavior and guard against regression. |
| marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java | Update property Javadoc to clearly document LOGGER/STDERR/stdout behaviors and LOGGER-mode category/level. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| * {@code INFO} level in your logging framework to enable network traffic logging; for example, | ||
| * in {@code logback.xml}: {@code <logger name="com.marklogic.client.okhttp.network" level="INFO"/>}. | ||
| */ | ||
| static final String NETWORK_LOGGER_NAME = "com.marklogic.client.okhttp.network"; |
There was a problem hiding this comment.
Couple thoughts - AFAICT, this was not a public feature. I can't find any evidence that it was documented - looking at https://docs.progress.com/bundle/marklogic-server-develop-with-java-11/page/topics/intro.html#id_45214 - and it wasn't in the Javadocs. So I think we're fine to no longer log this using the OkHttpServices logger at the debug level, which is good.
Second - I think we can forgo a package name here and use something easier to manage - I recommend "marklogic.okhttp.network". I think the word "network" is still useful to include, as it's a "network interceptor" and there might be future stuff we'd want to log about our usage of OkHttp that isn't specific to requests and responses.
I then think this constant should be in the public API somewhere so that users are encouraged to freely reference it. And really, the values for configuring this feature should be in the public API as well, as opposed to buried in OkHttpServices.
I'm thinking a new class in the com.marklogic.client.util package would be appropriate - LoggingUtil, perhaps? With at least these 3 values:
- The system property name for configuring the level of the OkHttp network logger - I think
marklogic.okhttp.network.levelis good (very friendly to e.g. Spring application files). - Also
marklogic.okhttp.network.outputfor selecting the logger, stdout, or stderr. - And then
marklogic.okhttp.networkas the name of the logger.
This is a useful feature, and making it public and documented like this will promote its usage (at a minimum, we'll be using it in the internal PDP project as it's very helpful for debugging).
There was a problem hiding this comment.
Just an FYI - you may like io.github.hakky54:logcaptor for tests that capture and assert on log messages. You can see it in action in marklogic-spark-connector - e.g.
try (LogCaptor logCaptor = LogCaptor.forName(Util.MAIN_LOGGER.getName())) {
But using the Logback APIs directly is fine too.
Summary
When
com.marklogic.client.okhttp.httplogginginterceptor.output=LOGGERwas set, network log lines were routed throughlogger.debug()on the logger categorycom.marklogic.client.impl.okhttp.RedactingHttpLogger. This forced users to enableDEBUGon an internal package just to see network traffic — exposing unrelated internal log output — and bound the category to an implementation class name that has no stability contract.This PR changes the LOGGER output path to a dedicated, stable category at
INFOlevel, with no other behavioral changes.Changes
RedactingHttpLogger.java
static final String NETWORK_LOGGER_NAME = "com.marklogic.client.okhttp.network"— the single source of truth for the new logger category.LoggerFactory.getLogger(RedactingHttpLogger.class)→LoggerFactory.getLogger(NETWORK_LOGGER_NAME). Using a string name rather than the class reference keeps the category stable across future refactors of the internal package hierarchy and places it outside theimpl.*namespace.logger.debug(redacted)→logger.info(redacted).NETWORK_LOGGER_NAMEconstant Javadoc to document the category, level, and an examplelogback.xmlconfiguration.RedactingHttpLoggerTest.java
loggerModeWritesToNetworkLoggerCategoryAtInfoLevel— attaches aListAppender<ILoggingEvent>tocom.marklogic.client.okhttp.network, invokeslog(), and asserts the captured event's logger name and level.loggerModeDoesNotRequireDebugOnImplPackage— setscom.marklogic.client.impl.okhttpto WARN and confirms the network event still arrives. Acts as a regression guard: if the logger were accidentally reverted togetLogger(RedactingHttpLogger.class), the WARN gate would suppress the INFO call and this test would fail.OkHttpServices.java
OKHTTP_LOGGINGINTERCEPTOR_OUTPUT— replaced "SLF4J debug logger" with a structured list documenting the new category, level, and all three output targets.Testing
All 11 tests in
RedactingHttpLoggerTestpass (9 pre-existing + 2 new), tested againstmarklogic-server-ubi:12.1.20260707-ubi-2.2.5:Notes
com.marklogic.client.okhttp.networkat INFO level (e.g.<logger name="com.marklogic.client.okhttp.network" level="INFO"/>inlogback.xml). No changes are needed toimpl.*logger configuration.Jira Ticket: MLE-29643