Add trace attributes and parser (trace_id, span_id and trace_flags)

[alexvanboxel]

This PR is to ask for feedback from the community. Locally I have a hacked OTEL collector running that uses the new trace attribute, transforms it to OTLP Log, and then pushes it to Google Logging.

I think this is the right place to introduce it as trace is a core component of OTLP.

If this is ok, I'll create tests and make it a proper commit.

@djaglowski

[alexvanboxel]

FYI: The end-to-end config I have success with (where this is part of):

config for Java (logback):

    <appender name="JSON" class="ch.qos.logback.core.rolling.RollingFileAppender">
        <file>${LOG_JSON_PATH}/xxx.jsonl</file>
        <rollingPolicy class="ch.qos.logback.core.rolling.SizeAndTimeBasedRollingPolicy">
            <fileNamePattern>${LOG_JSON_PATH}/dgc${LOG_FILE_NAME_PATTERN}.jsonl</fileNamePattern>
            <maxFileSize>10MB</maxFileSize>
            <maxHistory>5</maxHistory>
            <totalSizeCap>50MB</totalSizeCap>
        </rollingPolicy>

        <encoder class="net.logstash.logback.encoder.LogstashEncoder">
            <lineSeparator>UNIX</lineSeparator>
            <timestampPattern>[UNIX_TIMESTAMP_AS_NUMBER]</timestampPattern>
            <fieldNames>
                <timestamp>timestamp_ms</timestamp>
                <version>[ignore]</version>
                <levelValue>[ignore]</levelValue>
            </fieldNames>
        </encoder>
    </appender>

otel-collector

  filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: "json_parser"
        timestamp:
          parse_from: "timestamp_ms"
          layout_type: "epoch"
          layout: "ms"
        severity:
          parse_from: "level"
        trace:
          parse_trace_from: "trace_id"
          parse_span_from: "span_id"
          parse_flags_from: "trace_flags"

[djaglowski]

Looks great overall - just a couple questions

[djaglowski]

I don't have a lot of experience working with traces, so let me ask a very basic question:

Is it a safe assumption that TraceID, SpanID, and TraceFlags should always be hex encoded? In other words, is this implementation specific to any particular tracing technology or standard?

[tigrannajaryan]

Hex-encoding requirement and the specific names of the fields is part of Otel spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/overview.md#trace-context-in-legacy-formats

[djaglowski]

So this implementation will allow us to parse tracing data from logs that adhere to the OTel spec.

Certainly this is an improvement upon what is supported today, but isn't the purpose of the library to ingest logs from all sources?

I'm just trying to understand whether we are solving this in a generalized way, or if we are making an assumption about log format. Sounds like it's the latter, in which case I think we should consider this implementation preliminary, and plan to generalize it later.

Maybe something like:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: "json_parser"
        timestamp:
          parse_from: "timestamp_ms"
          layout_type: "epoch"
          layout: "ms"
        severity:
          parse_from: "level"
        trace_id:
          parse_from: "trace_id"
          encoding: "hex" // default is "hex", for now hardcoded as "hex"
        span_id:
          parse_from: "span_id"
          encoding: "hex" // default is "hex", for now hardcoded as "hex"
        trace_flags:
          parse_from: "trace_flags"
          encoding: "hex" // default is "hex", for now hardcoded as "hex"

[djaglowski]

I would even go so far as to say that the config in this PR needs to be changed in order to accomodate the above. We don't necessarily need to support the encoding parameter yet, but we should anticipate that it will be added later:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: "json_parser"
        timestamp:
          parse_from: "timestamp_ms"
          layout_type: "epoch"
          layout: "ms"
        severity:
          parse_from: "level"
        trace_id:
          parse_from: "trace_id"
        span_id:
          parse_from: "span_id"
        trace_flags:
          parse_from: "trace_flags"

[tigrannajaryan]

@djaglowski I agree. Let's just make the defaults compliant with Otel spec.

[alexvanboxel]

Mmm, in the suggested config the trace/span/flags are split, and if I understand it correctly that would mean 3 different operators? That also means that if you want to default you need to specify:

3 operators for defaults:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: "json_parser"
        trace_id: {}
        span_id: {}
        trace_flags: {}

1 operator for defaults:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: "json_parser"
        trace: {}

Anyone strong opinions?

[djaglowski]

I see your point about what I suggested. I think it can be done with 1 operator though, by pushing it down one level, as shown in your last suggestion:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: json_parser
        trace: {}

Which, accounting for default values would give an effective configuration:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: json_parser
        trace:
          trace_id:
            parse_from: trace_id
          span_id:
            parse_from: span_id
          trace_flags:
            parse_from: trace_flags

One could partially customize this, as in:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: json_parser
        trace:
          span_id:
            parse_from: my_custom_span_id

And this would also allow us to extend as necessary in the future:

filelog:
    include: [ /.../logs/*.jsonl ]
    operators:
      - type: json_parser
        trace:
          trace_id:
            parse_from: trace_id
            encoding: hex
          span_id:
            parse_from: my_custom_span_id
            encoding: md5
          trace_flags:
            parse_from: my_trace_flags_array
            encoding: array
            some_other_config_thing: my_value

The above may need some tweaking to make sense for traces, but my primary concern is ensuring that we can extend the design later to support trace parsing for non-OTel compliant logs.

[alexvanboxel]

Alright, that makes sense. I'll do it like that (skipping the customization for now, defaulting to the otel spec)

[alexvanboxel]

Rebased against main, squashed into a single commit. I think I took all the concerns into accounts and added tests.

[djaglowski]

This looks good. Thanks for the idea and implementation.

You've got a merge conflict with main due to #88, but that should be pretty straightforward to resolve.

[alexvanboxel]

Rebase and Squashed. Once it's merged and the log-collector is released, I'll need to wire up the extra fields in the otel-collector.

[djaglowski]

Thanks @alexvanboxel!