feat: Enable access to LogRecord in LoggingEnhancer (e.g., for access to flogger LogSite)

[bpcreech]

Is your feature request related to a problem? Please describe.

The LoggingEnhancer interface enables an implementer to add extra content to a LogEntry. However, the LoggingEnhancer is not itself given any context about the underlying LogRecord, so any added content must essentially be static and unrelated to the specific log record.

Some logging systems, like flogger, subclass java.util.logging.LogRecord (e.g., flogger makes a com.google.common.flogger.backend.system.AbstractLogRecord) and add extra information (e.g., in the case of flogger, the AbstractLogRecord has getLogData().getLogSite() which offers a much more efficient and less hassle-free mechanism of retrieving log site information than com.google.cloud.logging.SourceLocation.fromCurrentContext).

Describe the solution you'd like

Can we add a LogRecord parameter to LoggingEnhancer.enhanceLogEntry? This would be a breaking change; if it's too late for such a change, maybe we need a LoggingEnhancerWithRecord or LoggingEnhancer2.

Then as a follow-on, some library can supply the glue for flogger and this library: a stock LoggingEnhancer to supply the LogSite.

Describe alternatives you've considered

We currently have a logging handler which downcasts the LogRecord, but it has its own java.util.logging.LogHandler implementation and thus doesn't use LoggingHandler or the java-logging library. I'm filing this feature request in hopes of replacing that internal implementation.

Additional context

Contact me for the internal example of downcasting LogRecord. :)

[minherz]

Hi, the information from the LogRecord is translated to LogEntry:

java-logging/google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java

Lines 268 to 295 in c003417

	public void publish(LogRecord record) {
	// check that the log record should be logged
	if (!isLoggable(record)) {
	return;
	}
	// HACK warning: this logger doesn't work like normal loggers; the log calls are
	// issued
	// from another class instead of by itself, so it can't be configured off like
	// normal
	// loggers. We have to check the source class name instead.
	if ("io.netty.handler.codec.http2.Http2FrameLogger".equals(record.getSourceClassName())) {
	return;
	}
	LogEntry logEntry;
	try {
	logEntry = logEntryFor(record);
	} catch (Exception ex) {
	getErrorManager().error(null, ex, ErrorManager.FORMAT_FAILURE);
	return;
	}
	if (logEntry != null) {
	try {
	getLogging().write(ImmutableList.of(logEntry), defaultWriteOptions);
	} catch (Exception ex) {
	getErrorManager().error(null, ex, ErrorManager.WRITE_FAILURE);
	}
	}
	}

If there is any information that is not captured, a user can develop an enhancer and register it to enhance resulted LogEntry instance. It will help us to understand better your request if you can provide an example of the data that you are trying to log with JUL and what data you eventually expect to see in Log Explorer.

[bpcreech]

Right the problem is that the enhancer doesn't get any access to the LogRecord.

Here's specifically what I'm after: the flogger library subclasses LogRecord. I want to to implement an Enhancer which downcasts the LogRecord to get to the AbstractLogRecord, so I can call getLogData(), because there's a bunch of information there that isn't in the base LogRecord. Unfortunately, the Enhancer interface has no access to the original LogRecord, so this is not possible.

[minherz]

Do you effectively try customize the current logic of translating LogRecord to LogEntry that LoggingHandler class implements?

[bpcreech]

Yeah you could definitely put it that way.

Since this logic requires a dependency on the Google flogger library, I would suspect you don't want to do exactly that translation in this library, but the LogEnhancer interface is almost ideal for enabling the caller to add it. Almost.

[minherz]

We do not plan to introduce this level of customization at the moment.
Would you consider to inherit from com.google.cloud.logging.LoggingHandler and just override the current publish() implementation?

[bpcreech]

Sure! But I think we'd need to make logEntryFor protected (it's currently private) and/or refactor it out into its own reusable class.

[minherz]

We will look into refactoring the current implementation to support inheritance 👍

[minherz]

@bpcreech do you think that the FR in #32 is more flexible than one that you ask?

[bpcreech]

Oh, yeah, I didn't see that one! Looks like what we'd do with it, to slightly modify dansiviter@'s example, is:

public interface LoggingEventEnhancer extends com.google.cloud.logging.LoggingEventEnhancer<java.util.logging.LogRecord> {
   ...
}

[minherz]

I will close this issue then. We will work to implement #32. However, the change like that is a breaking change. It cannot be implemented without either creating a parallel flow for logs enhancing or by breaking the existing API. It means this change can be introduced only within the next major release.