Skip to content

Use shared springutils logger in RACM - #126

Open
glemson wants to merge 3 commits into
mainfrom
racm-logging-updates
Open

Use shared springutils logger in RACM#126
glemson wants to merge 3 commits into
mainfrom
racm-logging-updates

Conversation

@glemson

@glemson glemson commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Migrates RACM from its custom dual-logging setup to the shared springutils:logging-interceptor pattern, eliminating a redundant Logger instance.

Problem

RACM maintained its own parallel logging infrastructure alongside the spring-utils logging beans that were already active via component scan:

  • LoggingConfig.java — custom @ConfigurationProperties reading org.sciserver.racm.logging.*, duplicating what LoggerConfig (spring-utils) already provides.
  • LoggingInitializer.java — created a second Logger instance inside LogUtils, separate from the spring-managed bean.
  • LogUtils.java — static singleton holding its own Logger, with manual enabled guards.

The result was two Logger instances at runtime, with only the RACM-local one actually used for RACM/JOBM/FileService log messages.

Changes

Deleted

  • config/LoggingConfig.java — replaced by spring-utils LoggerConfig, which reads the standard logging.* properties.

Refactored

  • LogUtils.java — removed the singleton instance, setupLogger(), and the isLogInfoEnabled()/isLogErrorEnabled() guards. Now resolves the spring-managed Logger bean via Log.getLogger().
  • LoggingInitializer.java — reduced to a @Component that injects logging.jobm.application via @Value and sets it on LogUtils through a static setter. The @Value default (JOBM) matches the deleted class's DEFAULT_JOBM_APPLICATION_NAME, and Helm never overrode it, so no new Helm key is required.

Configuration

  • racm-application.yaml — removed the org.sciserver.racm.logging block (nothing binds it once LoggingConfig is gone) and added logging.rabbitmq.queuename. The logging.* section now matches fileservice-application.properties exactly, which is the other service already on this pattern.
  • application.properties (untracked, local dev) — uses the same standard logging.* keys.

Fixed

  • FileServiceRepository.java — replaced 2 System.out.printf calls with SLF4J LOG.warn().

Public API

LogUtils.buildLog() and the LogBuilder fluent API are unchanged. All 11 files using LogUtils.buildLog() required no modification.

Behavior change worth a reviewer's attention

Emitted log messages previously carried Host = "Kubernetes", set via the deleted config's application-host. LoggerConfig does not set applicationHost, so Logger.setupMessage() falls back to InetAddress.getLocalHost().getHostName() — the pod hostname.

This is deliberate: it matches fileservice. The services still emitting Host = Kubernetes (compute, logging-api, login-portal) do so through the legacy Log.ApplicationHost config-file path. Anyone with a saved query filtering RACM rows on Host = 'Kubernetes' will need to update it.

Testing

Unit testsLogUtilsTests adds 6 tests covering what this migration changed: which application name and message type each LogUtils path puts on the message.

Test Pins
jobmLogUsesConfiguredJobmApplicationName Application = configured JOBM name, MessageType.JOBM
racmLogKeepsLoggerApplicationName Application not overridden, MessageType.RACM
fileServiceLogKeepsLoggerApplicationName Application not overridden, MessageType.FILESERVICE
jobmErrorLogUsesConfiguredJobmApplicationName logError's isJOBM branch, MessageType.ERROR
racmErrorLogKeepsLoggerApplicationName logError without the JOBM override
loggerFailureDoesNotPropagateToCaller wrapSendingMessage swallows SendMessage failures

LogUtils resolves its Logger through the static ApplicationContext held by Log, so the tests populate that static with a mock context rather than starting a Spring context — no AspectJ weaving or EclipseLink enhancement needed, and therefore no -javaagent setup. A bare Logger has no sink enabled, so SendMessage does no I/O while a Mockito spy captures the built message.

loggerFailureDoesNotPropagateToCaller intentionally emits a "Failed to send message to sciserver logger" ERROR line on the console — that log line is the behavior under test, and the test is green when it appears.

./gradlew :racm:test passes: 22 tests, 0 failures (16 pre-existing across 5 classes, plus these 6). :racm:build and checkstyleTest pass at the existing warning baseline.

Sanity-checked against a mutation: commenting out message.Application = jobmApplicationName; in logJobm fails exactly one test with expected:<JOBM_TEST> but was:<RACM>, confirming the tests fail when the behavior breaks.

Smoke test — RACM was started in Eclipse against a local RACM database and the production login portal, and log messages were emitted with the expected Application, Host, and MessageType. This is the part unit tests deliberately cannot cover: the mock-ApplicationContext fixture bypasses real Spring wiring, so only a live run confirms that deleting LoggingConfig left no dangling injection point and that Log.getLogger() resolves.

Follow-ups (not in this PR)

  • A latent NPE: LogUtils.fillMessageWithUserInfo dereferences RequestContextHolder.getRequestAttributes() with no null check, and it now runs unconditionally rather than behind the removed enabled guard. Unreachable today — RACM has no @Async, @Scheduled, or manually created threads, so every LogUtils call is on a request thread — but it deserves a guard.
  • helm template verification: helm was not available in the dev environment used, so the chart edit is inspected but unrendered. Worth confirming in CI before deploy.

🤖 Generated with Claude Code

glemson and others added 3 commits July 31, 2026 14:25
RACM maintained its own logging infrastructure alongside the spring-utils
logging beans that were already active via component scan, resulting in two
Logger instances at runtime with only the RACM-local one actually used for
RACM/JOBM/FileService messages.

- Delete config/LoggingConfig.java; spring-utils LoggerConfig already reads
  the standard logging.* properties.
- LogUtils: drop the singleton, setupLogger(), and the isLogInfoEnabled()/
  isLogErrorEnabled() guards; resolve the spring-managed Logger bean via
  Log.getLogger(). SendMessage checks remoteEnabled/consoleEnabled/fileEnabled
  internally.
- LoggingInitializer: reduce to a @component injecting logging.jobm.application
  via @value and setting it through a static setter. The default matches the
  previous DEFAULT_JOBM_APPLICATION_NAME, so no new Helm key is needed.
- racm-application.yaml: remove the now-unbound org.sciserver.racm.logging
  block, add logging.rabbitmq.queuename, and match fileservice exactly.
- FileServiceRepository: replace two System.out.printf calls with LOG.warn().

LogUtils.buildLog() and the LogBuilder fluent API are unchanged; all 11
callers required no modification.

One production-visible change: emitted messages previously carried
Host = "Kubernetes" from the deleted config. LoggerConfig does not set
applicationHost, so Logger.setupMessage() now falls back to the pod hostname,
matching fileservice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers the behavior the springutils logger migration changed: which
application name and message type each LogUtils path puts on the message.

LogUtils resolves its Logger through the static ApplicationContext held by
Log, so the tests populate that static with a mock context instead of
starting a Spring context. A bare Logger has no sink enabled, so SendMessage
does no I/O and a Mockito spy captures the message that was built.

- jobmLogUsesConfiguredJobmApplicationName / jobmErrorLogUsesConfigured-
  JobmApplicationName pin the configured JOBM name, which is what replaced
  LoggingConfig.getJobmApplicationNameForLogger().
- racmLogKeepsLoggerApplicationName, fileServiceLogKeepsLoggerApplicationName
  and racmErrorLogKeepsLoggerApplicationName pin that the Application name is
  not overridden outside the JOBM paths.
- loggerFailureDoesNotPropagateToCaller pins that wrapSendingMessage swallows
  a SendMessage failure, so a logging problem cannot break a request. That
  test intentionally emits an ERROR line on the console.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@glemson
glemson marked this pull request as ready for review July 31, 2026 20:33
@glemson
glemson requested review from amitschang and Copilot and removed request for Copilot July 31, 2026 20:34

@amitschang amitschang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change-wise it looks fine to me. The log functions that are rewired are somewhat redundant now with the per-request injected logs (before this change), but I think in exception cases they miss some information due to exceptions getting eaten in endpoints. Something to look to change in the future probably.

logging.application: RACM
logging.rabbitmq.host: {{ include "sciserver.rabbitmq_host" . }}
logging.rabbitmq.exchange: {{ .Values.logging.rabbitmq.exchange }}
logging.rabbitmq.queuename: {{ .Values.logging.rabbitmq.queuename }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skeptical that this is needed, as it should be consumer related. But also shouldn't harm anything being here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants