serilog/serilog-sinks-opentelemetry

Option to send spans to the logs endpoint

Opened this issue · 3 comments

srogovtsev commented

serilog-sinks-opentelemetry/src/Serilog.Sinks.OpenTelemetry/Sinks/OpenTelemetry/OpenTelemetrySink.cs

Lines 50 to 71 in 2c9ebbc

public void Emit(LogEvent logEvent)
{
bool? isSpan = null;
if (_logsSink != null)
{
isSpan = IsSpan(logEvent);
if (!isSpan.Value)
{
_logsSink.Emit(logEvent);
return;
}
}
if (_tracesSink != null)
{
isSpan ??= IsSpan(logEvent);
if (isSpan.Value)
{
_tracesSink.Emit(logEvent);
}
}
}

If I understand the code correctly, if LogEvent is recognized as a span (i.e. it has SpanStartTimestamp property and some other things), it won't end up in logging even if tracing is not enabled - i.e., basically, lost.

I find this somewhat unexpected (but maybe I'm just reading it wrong), and I'd probably prefer this to be a configurable choice between

  • "send to traces if trace endpoint present, otherwise logs" (default)
  • "send to traces only, lost if not configured"
  • "always send to traces and logs" (useful when logging and tracing endpoint are not well coordinated)
nblumhardt commented

The configuration API is designed so that if the OTLP endpoints for logs and spans are in completely separate systems, with different auth requirements or protocols, two instances of the sink can be added to the logging pipeline - one with only the traces endpoint configured, and the other with the logs endpoint only. Having spans and log events on an equal footing (neither falls back to the other) fits nicely with this; I think changing behavior based on whether one or both endpoints are configured would add some complexity.

But, redirecting spans to the logs endpoint seems totally reasonable, too. The first solution that comes to mind would be to transform the spans into logs before they reach the sink:

class TracesToLogsEnricher: ILogEventEnricher
{
    public void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory)
    {
        if (logEvent.Properties.TryGetValue("SpanStartTimestamp", out var start))
        {
            logEvent.AddOrUpdateProperty(new LogEventProperty("StartTimestamp", start));
            logEvent.RemovePropertyIfPresent("SpanStartTimestamp");
        }
    }
}

And this could work the other way, too, if it's desirable for existing log events with timing information to be transformed into spans.

It's not an out-of-the-box solution but might be worth thinking about some more, so that a wider range of configurations can be handled using the same extension point 🤔

srogovtsev commented

While I do agree that changing the routing based on what is configured does seem overly complex (although I have in mind a particular scenario for that, but it can be addressed by other means for which I would probably provide another issue later), having the spans appear in the logs endpoint still seams reasonable to me. My reasoning here is simple: when we hook any other sink to the pipeline, it will see the spans as regular log entries (with SpanStartTimespan), so having them missing from the logs in telemetry would seem awkward just in terms of comparison. I think that the simplest and the most straightforward solution for this would be to simply provide another option, and I can draft a PR to that effect, if this is agreable.

nblumhardt commented

Thanks for the follow-up. An option like SendSpansToLogsEndpoint seems reasonable, but I'm hesitant to bake this in without a bit more time to size up the scenario, while a workaround exists. Let's leave this open to track it for a little while.

  • 👍1