monitor type builder

[ndr-brt]

Feature Request

Currently the monitor instance is passed to all the components that needs it, but it doesn't get "specialized", so if from a component an info is logged:

monitor.info("interesting information");

on the console will be printed without an indication of the component, this could make logs hard to read.
A current workaround is to manually add a prefix on the log message, but a more holistic solution could be to have a way to build a Monitor instance that will add a prefix, like

var componentMonitor = monitor.forComponent("Component");
new Component(monitor);

then every Component log will have the [Component] prefix

Which Areas Would Be Affected?

e.g., DPF, CI, build, transfer, etc.

Why Is the Feature Desired?

Are there any requirements?

Solution Proposal

If possible, provide a (brief!) solution proposal.

[jimmarino]

By "component," could we use something more meaningful than the class name since that is not very informative? For example, it could include the extension name and "service" or "subsystem" that the message is emitted from.

This could potentially be handled in the injector, which could create a wrapper implementation.

[ndr-brt]

By "component," could we use something more meaningful than the class name since that is not very informative? For example, it could include the extension name and "service" or "subsystem" that the message is emitted from.

This could potentially be handled in the injector, which could create a wrapper implementation.

Yes, I didn't specify it, the string will valued by the injector, so likely in the extensions's initialize method

[hamidonos]

I think the suggested solution works fine, although in my opinion it shouldn't be the monitors responsibility to create new specialized monitors for (extension) components.

Solution 1

The monitor is a singleton provided by the ServiceExtensionContext. We can build a decorator around the Monitor.
The decorator holds information about the context of the log so that
Registered Web API context alias: control
can become
[JerseyExtension]: Registered Web API context alias: control.

By using a decorator we can continue using the original monitor from the service extension context.

Inside the service extension we create the decorator like this:

var serviceExtensionMonitor = new ServiceExtensionMonitor(monitor);

The decorator could look like:

public class ServiceExtensionMonitor implements Monitor {

    private final Monitor monitor;
    private final String context;

    public ServiceExtensionMonitor(Monitor monitor) {
        this.monitor = monitor;
        this.context = getExtensionName();
    }

    // decorate methods of the actual monitor here

    private String getExtensionName() {
        return StackWalker
                .getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE)
                .walk(stackFrames -> stackFrames
                        .skip(1)
                        .filter(s -> SystemExtension.class.isAssignableFrom(s.getDeclaringClass()))
                        .findFirst()
                        .map(StackWalker.StackFrame::getDeclaringClass)
                        .map(Class::getSimpleName)
                        .orElse(null));
    }
}

The getExtensionName method will walk through the stacktrace to find the parent extension class. The operation is not very time consuming and since this is happening on application bootstrap it won't impact the running application.

By doing this we can avoid the manual configuration of the context where context = extension name.

Note that the context attribute holds the actual value to be prefixed on the log messages.

In some cases we don't want the extension name to be prefixed to the logs but rather the component name or some different value.

For example: All logs written by the TransferProcessManager should be prefixed by TransferProcessManager and not TransferCoreExtension.

This can be done by passing a string to the constructor of the decorator. In those cases the getExtensionName method will be ignored. Similar to what @ndr-brt suggested previously.

    public ServiceExtensionMonitor(Monitor monitor, String context) {
        this.monitor = monitor;
        this.context = context;
    }

var serviceExtensionMonitor = new ServiceExtensionMonitor(monitor, "TransferProcessManager");

Solution 2

It might also be desirable to prefix the class name of the component so that
[JerseyExtension]: Registered Web API context alias: control
could become
[JerseyExtension] [JerseyRestService]: Registered Web API context alias: control

This can come in handy where the monitor is passed further down the component tree of an extension. That way multiple components (or classes) inside an extension can be differentiated.

With Solution 1 this can only be achieved by stack walking through the stacktrace. This would have to take place inside the monitor implementation(s).
In my opinion we should not do that, because this will impact the application performance since stack walking has to be performed for every log operation. Furthermore it shouldn't be the monitors responsibility to resolve the caller class that way.

A better solution could be to have every component instantiate it's own decorator which also holds information about the components class while the extension name gets resolved automatically by stack walking.

    this.monitor = new ServiceExtensionMonitor(monitor, this.class.getName());

or

    this.monitor = new ServiceExtensionMonitor(monitor, "some-other-random-prefix");

This is again done on application start-up, meaning that there is no impact on application performance.

[ndr-brt]

I don't think we should bind it to the ServiceExtension, the majority of monitor calls are done in the services, not in the extensions, so I would call it differently, like PrefixMonitor, and remove that StackWalker logic.

I think the suggested solution works fine, although in my opinion it shouldn't be the monitors responsibility to create new specialized monitors for (extension) components.

We could leave to the users the discover of the existence of such a class, but a method on the Monitor interface that fluently permits would be more intuitive to use, like:

default Monitor withPrefix(String prefix) {
    return new PrefixMonitor(this, prefix);
} 

that could then be used this way:

var transferProcessManagerMonitor = context.getMonitor().withPrefix("TransferProcessManager");

[jimmarino]

I don't think we should bind it to the ServiceExtension, the majority of monitor calls are done in the services, not in the extensions, so I would call it differently, like PrefixMonitor, and remove that StackWalker logic.

I think the suggested solution works fine, although in my opinion it shouldn't be the monitors responsibility to create new specialized monitors for (extension) components.

We could leave to the users the discover of the existence of such a class, but a method on the Monitor interface that fluently permits would be more intuitive to use, like:

default Monitor withPrefix(String prefix) {
    return new PrefixMonitor(this, prefix);
} 

that could then be used this way:

var transferProcessManagerMonitor = context.getMonitor().withPrefix("TransferProcessManager");

I prefer this way as, in addition to being simpler, it decouples the prefix from the actual code artifact emitting the message. For example, what we shouldn't adopt is the traditional approach to logging where prefixes correspond to class names since that is virtually useless from an operations perspective.

[hamidonos]

Alright, I will go with @ndr-brt's suggested solution then

Keep in mind that with @ndr-brt's solution it is possible to chain prefixes, meaning that something like
DEBUG 2023-11-09T15:28:32.332917 [TransferCoreExtension] [TransferProcessManager] : Process 11fb870a-61d9-43a9-a1f5-bff1f74ff08e is now INITIAL
is possible

I will omit the Classname prefix for now unless you think otherwise

[jimmarino]

Alright, I will go with @ndr-brt's suggested solution then

Keep in mind that with @ndr-brt's solution it is possible to chain prefixes, meaning that something like DEBUG 2023-11-09T15:28:32.332917 [TransferCoreExtension] [TransferProcessManager] : Process 11fb870a-61d9-43a9-a1f5-bff1f74ff08e is now INITIAL is possible

I will omit the Classname prefix for now unless you think otherwise

This issue should only address the change @ndr-brt outlined and not include message refactorings of the codebase. Java class names don't make sense to include in messages (except in a stacktrace).

@ndr-brt we should discuss creating a DR for how prefixes should be defined. I prefer the concept of a "component" or "subsystem" that maps to a discrete unit of code artifacts.