Add support for Socket Logging Handler with basic ECS format logging #43232

troosan · 2024-09-12T06:43:13Z

This pull request is a followup of PR #23128 and is greatly inspired by the following provided by @geoand https://github.com/quarkusio/quarkus/compare/main...geoand:%2323127?expand=1

It allows sending logs in ECS format to a socket (typically a logstash tcp input)

fixes #23127 and answers my own question on StackOverFlow

troosan · 2024-09-12T06:45:22Z

I'm still working on updating the documentation for it (logging and centralized-log-management)

The logging one is easy, the centralized log management is more complicated, as it promotes the use of the GELF format.
I think I'll just add a section under "GELF alternative: use Syslog" for now.

But I believe logging in ECS format should be promoted over the GELF format as it fits better in the ELK stack

geoand · 2024-09-12T10:19:49Z

Looks good, thanks!

@dmlloyd any comments?

github-actions · 2024-09-12T10:52:06Z

🙈 The PR is closed and the preview is expired.

troosan · 2024-09-12T12:51:16Z

The resulting json is the following

{
            "threadId": 449,
           "timestamp": "2024-09-12T08:56:17.609151+02:00",
          "@timestamp": 2024-09-12T06:56:17.610853593Z,
          "threadName": "Shutdown thread",
             "message": "my-app stopped in 0.118s",
                 "ndc": "",
            "sequence": 4335,
     "loggerClassName": "org.jboss.logging.Logger",
          "loggerName": "io.quarkus",
         "processName": "/Users/antoine/.sdkman/candidates/java/17.0.9-graalce/bin/java",
           "processId": 65262,
               "level": "INFO",
            "@version": "1",
                "mdc": {},
            "hostName": "macbook-m1-pro.localdomain"
}

I guess it's a good start, but we could add default values for parameters as serviceName, serviceVersion, serviceEnvironment, ...

https://www.elastic.co/guide/en/ecs-logging/java/1.x/setup.html#_add_the_dependency

or we let the generation of an ECS compatible json to the https://github.com/quarkiverse/quarkus-logging-json/blob/main/runtime/src/main/java/io/quarkiverse/loggingjson/LoggingJsonRecorder.java#L92 extension.

I'm not sure why there are 2 impl for json logging actually.

troosan · 2024-09-13T09:46:35Z

@dmlloyd @geoand should we natively provide an valid ECS json?

using an external library (provided by elastic) It looks quite lightweight: https://github.com/elastic/ecs-logging-java/tree/main/jboss-logmanager-ecs-formatter
by building the json manually

or should I update the documentation to mention the quarkiverse version of the quarkus-logging-json, which supports ECS encoding. This will need a PR for that extension though to enable using the socket handler.

geoand · 2024-09-13T10:14:49Z

@loicmathieu you've used the logging-json stuff IIRC, what's your take?

loicmathieu · 2024-09-13T11:11:49Z

quarkus-logging-json is the official way to log in JSON.

What I don't understand is why it mandates modification in the logging-json library, socket loggings should be a log handler so it should work with all supported log format/pattern?

By the way, logging-gelf already support that.

troosan · 2024-09-13T12:21:17Z

I think the reason this required a change in the logging-json is because it's configured in a different way.
The json logging can be configured per handler (console, file and now socket) whereas the gelf handler handles only it's own format.
Maybe indeed this should be reversed, and the socket handler should allow only the ECS format, just as the GELF handler is allowing only the GELF format.

I know the gelf logging already supports sending logs to logstash, but my operations team is not too keen on enabling & maintaining another input (and the filters that go with it) in the logstash pipeline.

loicmathieu · 2024-09-13T13:01:40Z

OK then.

troosan · 2024-09-16T07:24:58Z

@geoand @gsmet Contrary to what I put int the documentation, with this current PR, the format of the JSON does not follow the ECS standards.

For now I have gone around this by setting field key overrides through the configuration, hiding/adding additional fields, like this:

quarkus.log.socket.json.exception-output-type=formatted
quarkus.log.socket.json.key-overrides=timestamp=@timestamp,\
	logger_name=log.logger,\
	level=log.level,\
	thread_name=process.thread.name,\
	thread_id=process.thread.id,\
	host_name=host.hostname,\
	stack_trace=error.stack_trace
quarkus.log.socket.json.excluded-keys=sequence,loggerClassName
quarkus.log.socket.json.additional-field."service.environment".value=${quarkus.profile}
quarkus.log.socket.json.additional-field."service.name".value=${quarkus.application.name}
quarkus.log.socket.json.additional-field."service.version".value=${quarkus.application.version}
quarkus.log.socket.json.additional-field."data_stream.type".value=logs
quarkus.log.socket.json.additional-field."ecs.version".value=1.12

This is only however not a very clean solution, I could also do this programmatically like this in the LoggingJsonRecorder

    public RuntimeValue<Optional<Formatter>> initializeSocketJsonLogging(JsonLogConfig config) {
        if (config.socketJson.logFormat == JsonConfig.LogFormat.ECS) {
            addEcsFields(config.socketJson);
        }
        return getFormatter(config.socketJson);
    }

    private void addEcsFields(JsonConfig config) {
        EnumMap<Key, String> keyOverrides = PropertyValues.stringToEnumMap(Key.class, config.keyOverrides.orElse(null));
        keyOverrides.putIfAbsent(Key.TIMESTAMP, "@timestamp");
        keyOverrides.putIfAbsent(Key.LOGGER_CLASS_NAME, "log.logger");
        keyOverrides.putIfAbsent(Key.LOGGER_NAME, "log.logger");
        keyOverrides.putIfAbsent(Key.LEVEL, "log.level");
        keyOverrides.putIfAbsent(Key.PROCESS_ID, "process.pid");
        keyOverrides.putIfAbsent(Key.PROCESS_NAME, "process.name");
        keyOverrides.putIfAbsent(Key.THREAD_NAME, "process.thread.name");
        keyOverrides.putIfAbsent(Key.THREAD_ID, "process.thread.id");
        keyOverrides.putIfAbsent(Key.HOST_NAME, "host.hostname");
        keyOverrides.putIfAbsent(Key.SEQUENCE, "event.sequence");
        keyOverrides.putIfAbsent(Key.EXCEPTION_MESSAGE, "error.message");
        keyOverrides.putIfAbsent(Key.STACK_TRACE, "error.stack_trace");
        config.keyOverrides = Optional.of(PropertyValues.mapToString(keyOverrides));

        config.additionalField.computeIfAbsent("ecs.version", k -> buildFieldConfig("1.12.2", Type.STRING));
        config.additionalField.computeIfAbsent("data_stream.type", k -> buildFieldConfig("logs", Type.STRING));

        quarkusConfig.getOptionalValue("quarkus.application.name", String.class).ifPresent(
                s -> config.additionalField.computeIfAbsent("service.name", k -> buildFieldConfig(s, Type.STRING)));
        quarkusConfig.getOptionalValue("quarkus.application.version", String.class).ifPresent(
                s -> config.additionalField.computeIfAbsent("service.version", k -> buildFieldConfig(s, Type.STRING)));
        quarkusConfig.getOptionalValue("quarkus.profile", String.class).ifPresent(
                s -> config.additionalField.computeIfAbsent("service.environment", k -> buildFieldConfig(s, Type.STRING)));
    }

which is not very clean either.

Also missing is the addition of fields like trace.id and span.id instead (or in addition to) of having them in the mdc.traceId and mdc.spanId

So question is, shall I commit this working (but far from perfect) solution and adapt the documentation accordingly?
Or do I just specify in the documentation how to make it ECS compatible through properties?

geoand · 2024-09-17T07:23:21Z

Or do I just specify in the documentation how to make it ECS compatible through properties?

I think this is best for now. @loicmathieu WDYT?

troosan · 2024-09-19T11:39:03Z

@geoand in case we go for the code solution instead of the properties solution, do you know how I can get the traceId and spanId? Those will obviously only be available if the OpenTelemetry dependency is present. Is there a way to retrieve those two values without relying on the io.opentelemetry.context classes?
I would like to add those in the ECS tracing fields
My current solution is to copy them in the Logstash pipeline with the following filter but it would be nicer to natively support it.

filter {
  if ![span][id] and [mdc][spanId] {
    mutate { rename => { "[mdc][spanId]" => "[span][id]" } }
  }
  if ![trace][id] and [mdc][traceId] {
    mutate { rename => {"[mdc][traceId]" => "[trace][id]"} }
  }
}

geoand · 2024-09-19T11:42:59Z

@geoand in case we go for the code solution instead of the properties solution, do you know how I can get the traceId and spanId? Those will obviously only be available if the OpenTelemetry dependency is present. Is there a way to retrieve those two values without relying on the io.opentelemetry.context classes?

@brunobat can give you the best way to access those, although in order to make reduce unnecessary couplinh, we'd need to put some kind of SPI in place.

brunobat · 2024-09-19T13:02:42Z

@troosan You need access to the context like the MDCEnabledContextStorage does:

quarkus/extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/MDCEnabledContextStorage.java

Line 30 in bbd821a

OpenTelemetryUtil.setMDCData(beforeAttach, null);

But this data is maintained by us in the MDC context, therefore, your solution sounds ok.

loicmathieu · 2024-09-19T13:09:09Z

I think we should have a util for that that can extract the spanId/transactionId if present as we need to do it for all handlers. For ex it would be useful to be able to forward these ids to Google Cloud Loggin, today it must be done manually by implementing a trace extractor: https://docs.quarkiverse.io/quarkus-google-cloud-services/main/logging.html#_tracing

This is a cross-extension concern so we should have something in the core that didn't depends on OTEL.

@troosan

Or do I just specify in the documentation how to make it ECS compatible through properties?

Yes, again we may want to provide something that will work cross handler, for example this is the code we have for Google Cloud Logging: https://github.com/quarkiverse/quarkus-google-cloud-services/blob/main/logging/runtime/src/main/java/io/quarkiverse/googlecloudservices/logging/runtime/ecs/EscJsonFormat.java

troosan · 2024-09-19T14:29:48Z

@loicmathieu that is indeed exactly what we need, Ideally the generation of the json (ECS or not) and where to send it (Google, Logstash, ...) should be decoupled. Otherwise we're just duplicating code all over.

brunobat · 2024-09-19T15:02:03Z

This is a cross-extension concern so we should have something in the core that didn't depends on OTEL.

But all tracing is currently impl by OTel...
What we can have is some service you can inject and get the Otel tracing data in a simple way.

troosan · 2024-09-23T09:38:49Z

@geoand I have updated the documentation on how to make the json ECS compatible through properties.
https://quarkus-pr-main-43232-preview.surge.sh/version/main/guides/centralized-log-management#logstash_ecs

Do you think this could get merged as is in next iteration? (I can squash the commits before merging)

Longer term the logging facility probably needs to be reviewed to avoid unnecessary duplication across the extensions.

gsmet

I added some minor suggestions and questions.

Other than that, I think the patch looks good and it's a very nice contribution.

core/runtime/src/main/java/io/quarkus/runtime/logging/SocketConfig.java

docs/src/main/asciidoc/centralized-log-management.adoc

docs/src/main/asciidoc/logging.adoc

core/runtime/src/main/java/io/quarkus/runtime/logging/LogConfig.java

dmlloyd · 2024-09-25T16:16:32Z

core/runtime/src/main/java/io/quarkus/runtime/logging/SocketConfig.java

+import io.quarkus.runtime.annotations.ConfigItem;
+
+@ConfigGroup
+public class SocketConfig {


Sorry for the late review, but if this is being used specifically to talk to one kind of service, then should we be having a general socket config? I think that users might have a harder time configuring a generic socket handler to talk to some service than if they could have a configuration specific to the kind of service they're talking to.

Theoretically the socket to which you are writing is independent of the format in which you send the data to it.
The current setup will allow you to format the logs however you want, JSON is only one of those.

The Socket handler should allow you to send logs to Splunk too for instance. I've not tried it out though.

dmlloyd · 2024-09-25T16:17:02Z

core/deployment/src/main/java/io/quarkus/deployment/builditem/LogSocketFormatBuildItem.java

+ * socket logging formatting configuration and use the formatter provided instead. If multiple formatters
+ * are enabled at runtime, a warning message is printed and only one is used.
+ */
+public final class LogSocketFormatBuildItem extends MultiBuildItem {


Won't this break if we have more than one socket handler configured for some reason?

you mean that we should be able to work with "named" handlers? To allow writing logs to 2 different socket appenders? (Logstash and Splunk for instance)

I must admit I don't know. This is just a copy of LogFileFormatBuildItem for instance

troosan · 2024-10-06T08:32:16Z

I don't really like the configuration. It seems over the top. I would instead rather be able to configure a handler for logstash or splunk, by name, and point it at a socket address and have that be the extent of the configuration necessary.

That would be another way to do it, but to be consistent you would need to change the whole way logging is configured right now. The elastic json log format (ECS) can be used for (using filebeat for file based appenders)

console
file
socket
If I understand correctly what you are suggesting you would prefer to configure first the format, then the handler, so quarkus.logging.elastic.console instead of quarkus.logging.console.elastic, which is the other way around than what is done now. That would be a major breaking change though ...

I've lost this battle before in a big way (data source URLs...) so if you want to override my objection go ahead. I would just prefer to have fewer low level details in the configuration, because avoiding that stuff is supposed to be a part of the value we add.

troosan · 2024-10-14T20:07:40Z

I guess everybody was in Antwerp at Devoxx last week :-)
Any chance to have this part of the next iteration?

dmlloyd · 2024-10-15T16:18:28Z

docs/src/main/asciidoc/centralized-log-management.adoc

+quarkus.log.socket.json.key-overrides=timestamp=@timestamp,\
+	logger_name=log.logger,\
+	level=log.level,\
+	process_id=process.pid,\
+	process_name=process.name,\
+	thread_name=process.thread.name,\
+	thread_id=process.thread.id,\
+	sequence=event.sequence,\
+	host_name=host.hostname,\
+	stack_trace=error.stack_trace
+quarkus.log.socket.json.excluded-keys=loggerClassName
+quarkus.log.socket.json.additional-field."service.environment".value=${quarkus.profile}
+quarkus.log.socket.json.additional-field."service.name".value=${quarkus.application.name}
+quarkus.log.socket.json.additional-field."service.version".value=${quarkus.application.version}
+quarkus.log.socket.json.additional-field."data_stream.type".value=logs
+quarkus.log.socket.json.additional-field."ecs.version".value=1.12.2


This I just don't like. Could we at least have a enum property on the JSON formatter for format presets? Asking every user to provide all of this each time is not OK IMO.

that's indeed what I proposed here: #43232 (comment)
@dmlloyd let me know what you prefer

troosan · 2024-10-23T10:51:21Z

looks like the move of the logging to @ConfigMapping made a mess out of my PR. I'll update it.
@geoand @dmlloyd @gsmet what do you prefer concerning #43232 (comment) ?

troosan · 2024-10-30T14:49:15Z

@dmlloyd what shall I do for this PR? I personally would tend to agree with you that it's much easier for the enduser to just specify quarkus.log.socket.json.log-format=ECS and do the key-overrides programmatically.
Once someone comes up with a "clean" way to produce the ECS formatted json, we can just swap the impl. The config key can stay the same, so it would be backward compatible.

dmlloyd · 2024-10-30T14:59:20Z

@dmlloyd what shall I do for this PR? I personally would tend to agree with you that it's much easier for the enduser to just specify quarkus.log.socket.json.log-format=ECS and do the key-overrides programmatically. Once someone comes up with a "clean" way to produce the ECS formatted json, we can just swap the impl. The config key can stay the same, so it would be backward compatible.

Sure, I like this approach. That would cover my two concerns (configurability, plus being able to clean up things in the future).

quarkus-bot · 2024-11-03T20:29:30Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit 3d110d1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot · 2024-11-04T01:37:59Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 3d110d1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

troosan · 2024-11-04T09:46:33Z

@dmlloyd changes done and documentation updated accordingly

dmlloyd

Looks OK. I think we'll probably want to revisit some of these internals at some point, but you've really done your best to accommodate and I think the current state is probably acceptable for now. Thanks.

troosan · 2024-11-05T19:18:26Z

you're welcome. I really need to dive into the quarkus build specificities to understand better how all this works !

cescoffier · 2024-11-08T17:49:45Z

@loicmathieu @dmlloyd Are we good on this one?

dmlloyd · 2024-11-08T18:03:15Z

I'm good with it

cescoffier · 2024-11-08T18:34:41Z

Thanks @dmlloyd !

loicmathieu · 2024-11-08T18:39:14Z

Good for me.

I'll open a discussion so review all ou centralized log solutions now that we have 3 of them: socket appender, logstash gelf and Open telemetry logs we would want to document the differences and maybe deprecate the old logstash gelf.

I'm also have a look if I can reuse the new ECS formatter in Google Cloud Logging

quarkus-bot bot added area/core area/logging labels Sep 12, 2024

troosan force-pushed the #23127 branch from 533041a to 3fd44c6 Compare September 12, 2024 07:50