-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] MetaDataDictionaryConverter - NullReferenceException #219
Comments
Maybe the Maybe |
Think these usual suspects should never be passed to
So any type matching the above types, should be converted directly to string. |
Tried to make a custom build of MetaDataDictionaryConverter that recognized the dangerous types above, and it helped a lot, but there were still some warnings when running ASP.NET6 application:
|
I think we are hitting this bug in System Text Json: dotnet/runtime#58690 Which won't get fixed until 7.0. Which is due out in november: https://github.com/dotnet/runtime/milestone/82 |
In my opinion the MetaDataDictionaryConverter should behave like NLog which supports the Message Templates standard when formatting placeholders in messages (see https://github.com/NLog/NLog/wiki/How-to-use-structured-logging#formatting-of-the-message). By default or when using the logger.LogInformation("Hello, I am {person}", new Person("John", "Doe"));
logger.LogInformation("Hello, I am {$person}", new Person("John", "Doe")); ... metadata should be serialized by calling the {
"@timestamp": "2022-11-28T13:58:21.4222655+01:00",
"log.level": "Info",
"message": "Hello, I am Person { FirstName = John, LastName = Doe }",
"metadata": {
"person": "Person { FirstName = John, LastName = Doe }"
},
... [removed for brevity]
} But in the current version 1.5.3 of Elastic.CommonSchema.NLog metadata is always serialized as JSON: {
"@timestamp": "2022-11-28T13:58:21.4222655+01:00",
"log.level": "Info",
"message": "Hello, I am Person { FirstName = John, LastName = Doe }",
"metadata": {
"person": {
"first_name": "John",
"last_name": "Doe"
}
},
... [removed for brevity]
} Only when the logger.LogInformation("Hello, I am {@person}", new Person("John", "Doe")); ... I'd expect the metadata value to be serialized as JSON: {
"@timestamp": "2022-11-28T13:59:07.6100035+01:00",
"log.level": "Info",
"message": "Hello, I am {\"FirstName\":\"John\", \"LastName\":\"Doe\"}",
"metadata": {
"person": {
"first_name": "John",
"last_name": "Doe"
}
},
... [removed for brevity]
} This allows the creator of the log message to determine whether or not to serialize the log arguments. Maybe we can reuse NLog's code for serializing metadata? |
I tried #222. It works great 👏 Can we get this merged? |
Starting ASP.NET6 Application with NLog EcsLayout where it fails to serialize object of type
Microsoft.AspNetCore.Routing.RouteEndpoint
coming from this log-message from Microsoft ASP.NET (Logger-Category = "Microsoft.AspNetCore.Routing.EndpointMiddleware"):Exception Details (Looks like it fails with a nested-value of type
Microsoft.AspNetCore.Routing.Patterns.RoutePattern
):The text was updated successfully, but these errors were encountered: