ecs-dotnet icon indicating copy to clipboard operation
ecs-dotnet copied to clipboard

[FEATURE] add "level" to the "log" object in addition to the top level "log.level"

Open johnjaylward opened this issue 4 years ago • 9 comments

Is your feature request related to a problem? Please describe. In ECS there is no top level "log.level" property. Generally in Elastic, this doesn't seem to pose an issue as keying works the same when using "log.level" or "log": { "level": ...}. However, this does not match the ECS specification and when pulling the documents themselves, the "log.level" is not in the "right" place.

Describe the solution you'd like Possibly post the log level in both fields like so:

{
    "@timestamp": "2020...",
    "message": "my log message",
    "log.level": "ERROR",
    "log": {
        "level": "ERROR",
...
}

Describe alternatives you've considered Don't post the top level "log.level" key at all, and just use the object key

Additional context see #77 for past discussion.

johnjaylward avatar Jun 03 '20 15:06 johnjaylward

I was hoping for a update on this enhancement request....??

ekwebster avatar Jan 25 '21 05:01 ekwebster

Thanks for opening @johnjaylward.

I've read through #77 and am familiar with the ECS logging specification. I'd like to understand more about the use case(s) the proposal might serve; grepping log files is one mentioned in #77, but are there others?

russcam avatar May 28 '21 03:05 russcam

TBH this was so long ago, I had to read through #77 to see why I even opened this.

If I recall my thinking at the time, the enhancement request was mostly for sending the information in the recommended ECS format. As someone who was new to Elastic, I was thrown off on why my documents did not match the ECS specification when using the Elastic provided ECS configuration.

I've since moved away from the ECS plugin and instead I use a custom built JSON payload configuration in NLog, mostly to get away from "odd" things like this that don't work as I expected, or that I didn't feel matched the ECS spec.

johnjaylward avatar May 28 '21 04:05 johnjaylward

@russcam winlogbeat and other beats are all using "level" nested under the "log" so I think it's time .NET EscLayout is updated as well.

It's also part of the base schema, but gets omitted for some strange reason.

It's also part of the official docs.

They can't just keep ignoring this thread forever. :-)

dmumladze avatar Feb 23 '22 20:02 dmumladze

As we have updated our code generation it seems we now emit log level twice as requested e.g:

{"@timestamp":"2022-09-06T21:17:25.2224365+02:00","log.level":"Information","message":"My log message!","metadata":{"MessageTemplate":"My log message!"},"ecs":{"version":"v8.4.0"},"event":{"created":"2022-09-06T21:17:25.2224365+02:00","severity":2,"timezone":"Central European Standard Time"},"log":{"level":"Information","logger":"Elastic.CommonSchema.Serilog.Tests.OutputTests"},"process":{"executable":"dotnet","name":"dotnet","pid":1159470,"thread.id":16}}

@z1c0 @gregkalapos @felixbarny thoughts if we should revert this and ensure we only emit log.level?

I'm personally not opposed to leaving it the way it is now.

Mpdreamz avatar Sep 06 '22 19:09 Mpdreamz

Emitting it only once as log.level definitely seems to be the cleaner solution to me. Since we already introduced breaking changes in https://github.com/elastic/ecs-dotnet/pull/211, it might be a good opportunity to make this change as well.

z1c0 avatar Sep 07 '22 05:09 z1c0

What's the impact that you see when the dotted notation is used?

Note that you can always use the dot_expander processor in Elasticsearch to convert the dotted field names into objects.

The reason why log.level is special-cases is to make ECS logs a bit more human-readable. As dots and nested object notations are mostly handled equivalently in Elasticsearch, it shouldn't matter too much. But I'm keen on hearing about use cases where this causes friction. My assumption would be these use cases can be solved via the dot_expander processor but maybe I'm missing something.

felixbarny avatar Sep 07 '22 06:09 felixbarny

Sorry this is not to question log.level being written first to comply with ECS logging spec.

We've always done so and will continue to do so.

The ask here is to duplicate it under log { level } too. Something we are not doing today in any released version but after updating our code generator ahead of the next release we are writing it twice.

Do we treat this as a bug or a feature (this request).

ECS only cares that log.level ends up in ES so either way would be compliant.

One reason for duplication is that deserializing _source needs special care to be aware of log.level existing at the root that needs carrying over to the instantiated log .NET object and visa versa.

If we duplicate it's easier to use other/default serializes on these types too.

Mpdreamz avatar Sep 07 '22 06:09 Mpdreamz

I don't have a strong opinion either way. Duplicating feels a bit off but it shouldn't have a negative impact.

IINM, Elasticsearch will de-duplicate the values in a field anyway so that it won't store log.level: ["ERROR", "ERROR"] but only log.level: ["ERROR"].

felixbarny avatar Sep 07 '22 09:09 felixbarny

Just to highlight things that it can beak. When the json.expand_keys option is used in filebeat, it will cause the following error:

cannot expand "log.level": found existing (string) value

elan-slovelock avatar Dec 06 '22 16:12 elan-slovelock