elastic / ecs-dotnet

https://www.elastic.co/guide/en/ecs-logging/dotnet/current/setup.html
Apache License 2.0
117 stars 59 forks source link

[BUG] Data mapping with Elastic Common Schema fail #402

Closed waldo2188 closed 1 week ago

waldo2188 commented 3 months ago

ECS integration/library project(s) (e.g. Elastic.CommonSchema.Serilog): Elastic.Serilog.Sinks ECS schema version (e.g. 1.4.0): The last one (v8.11.0) ECS .NET assembly version (e.g. 1.4.2): v8.11.0 Elasticsearch version (if applicable): v8.14.2 .NET framework / OS: Docker Linux with .net 8 Description of the problem, including expected versus actual behavior:

Hello,

I’m trying to replace the previous Serilog Sink Serilog.Sinks.Elasticsearch with the new one Elastic.Serilog.Sinks.

During our process with add elements in LogContext with LogContext.PushProperty().

When I use LogContext.PushProperty("client.nat.ip", “0.0.0.1”); the value is well set in the Elasticsearch CommonSchema in the client object, but this one no: LogContext.PushProperty("client.ip", “0.0.0.1”);. client.ip is added in the metadata object.

Another one, if I set LogContext.PushProperty("client.user.id", "regis"); no log are sent to elasticsearch. So, it has to fail somewhere.

Steps to reproduce:

  1. Add LogContext.PushProperty("client.ip", “0.0.0.1”);` in a controller or a service
  2. Add LogContext.PushProperty("client.user.id", "regis"); in a controller or a service

Is there a way to me to write tests in Elastic.CommonSchema.Serilog.Tests project ?

waldo2188 commented 3 months ago

I found why "client.user.id" key wasn't processed (but not why it silently crash...) I start by adding in class Elastic.CommonSchema.LogTemplateProperties :

// [...]
public static string ClientUserId = nameof(ClientUserId);
// [...]
public static readonly HashSet<string> All = new()
{
// [...]
    "client.user.id", ClientUserId,
// [...]
}

And next in class Elastic.CommonSchema. EcsDocument, I add case "client.user.id" in the right part of the switch case and manage it in the method called TrySetClient, like bellow :

public static bool TrySetClient(EcsDocument document, string path, object value)
{
    Func<Client, object, bool> assign = path switch
    {
// [...]
        "client.user.id" => static (e, v) => TrySetString(e, v, static (ee, p) => (ee.User ?? (ee.User = new User())).Id = p),
        _ => null
    };
    if (assign == null) return false;

    var entity = document.Client ?? new Client();
    var assigned = assign(entity, value);
    if (assigned) document.Client = entity;
    return assigned;
}

Is that the right way ? By the way I add a test method in namespace Elastic.CommonSchema.Serilog.Tests.Repro

public class GithubIssue402 : LogTestsBase
{
    public GithubIssue402(ITestOutputHelper output) : base(output) { }

    [Fact]
    public void Reproduce() => TestLogger((logger, getLogEvents) =>
    {
        using (LogContext.PushProperty("client.nat.ip", "10.1.2.3"))
        using (LogContext.PushProperty("client.ip", "11.2.3.4"))
        using (LogContext.PushProperty("client.user.id", "regis"))
            logger.Information("Logging something with log context");

        var logEvents = getLogEvents();
        logEvents.Should().HaveCount(1);

        var ecsEvents = ToEcsEvents(logEvents);

        var (_, info) = ecsEvents.First();
        info.Message.Should().Be("Logging something with log context");

        info.Client.NatIp.Should().Be("10.1.2.3");
        info.Client.Ip.Should().Be("11.2.3.4");
        info.Labels.Should().NotContainKey("client.user.id");
        info.Client.User.Id.Should().Be("regis");

    });
}

For the part of the LogContext with "client.ip" key, it’s work in the main git branch...

waldo2188 commented 1 week ago

Thanks !