Skip to content
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

Use Zipkin sanitizers #2164

Closed
pavolloffay opened this issue Feb 13, 2020 · 6 comments
Closed

Use Zipkin sanitizers #2164

pavolloffay opened this issue Feb 13, 2020 · 6 comments

Comments

@pavolloffay
Copy link
Member

The sanitizers https://github.com/jaegertracing/jaeger/tree/master/cmd/collector/app/sanitizer could be implemented as processors if we think they are optional https://github.com/open-telemetry/opentelemetry-collector#processors

@pavolloffay
Copy link
Member Author

pavolloffay commented Mar 27, 2020

So service_name_sanitizer and utf8_sanitizer are not installed by default.

Jaeger core only uses Zipkin sanitizers, that operate on zipkin spans https://github.com/jaegertracing/jaeger/blob/master/cmd/collector/app/sanitizer/zipkin/span_sanitizer.go

@pavolloffay
Copy link
Member Author

Otel collector might be doing some sanitization on zipkin spans when zipkin spans are transformed to OTEL model. We will have to test this.

@pavolloffay pavolloffay changed the title Use sanitizers before storing Use Zipkin sanitizers Mar 27, 2020
@pavolloffay pavolloffay transferred this issue from jaegertracing/jaeger-opentelemetry-collector Apr 6, 2020
@ghost ghost added the needs-triage label Apr 6, 2020
@pavolloffay
Copy link
Member Author

pavolloffay commented Apr 15, 2020

Errors

Errors are not recognized properly when for errors like span.tag("error", "error type or message")

    @RequestMapping("error_span")
    public String errorSpan() {
        zipkinTracing.tracing().tracer().newTrace()
            .start()
            .name("error_span")
            .tag("error", "some error")
            .finish();
        return "error_span_created";
    }

jaeger OTEL collector: https://i.imgur.com/PdWHkIn.png
all-in-one (with sanitizer): https://i.imgur.com/9ZjHKFt.png

Duration

Zipkin spans might have duration set to 0. In this case Jaeger derives the duration from annotations.

    @RequestMapping("missing_duration")
    public String missingDuration() {
        Span span = zipkinTracing.tracing().tracer().newTrace()
            .name("missing_duration")
            .start(0)
            .annotate("cs");
        span.annotate(zipkinTracing.tracing().clock(span.context()).currentTimeMicroseconds() + 1500, "cr");
        span.finish(0);
        return "missing_duration";
    }

Jaeger OTEL collector: https://i.imgur.com/ebs0tP5.png - the duration seems to be set correctly.
Jaeger all-in-one: https://i.imgur.com/w9FK1mz.png

@pavolloffay
Copy link
Member Author

pavolloffay commented Apr 16, 2020

The error semantic conventions haven't been yet defined at the spec level open-telemetry/opentelemetry-specification#427, open-telemetry/opentelemetry-specification#432 and open-telemetry/opentelemetry-specification#521

Once it's done we can correctly convert zipkin errors to OTEL model. If it deviates from OT we will implement a new proper mapping in Jaeger.

@pavolloffay
Copy link
Member Author

I have submitted a proposal to OTEL spec that includes behavior of zipkin sanitizers open-telemetry/opentelemetry-specification#566

@pavolloffay
Copy link
Member Author

Closing as it is a "low level" issue related to V2 which might take a different approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant