Replace OpenTracing with OpenTelemetry

[sagikazarmark]

Is your feature request related to a problem? Please describe.
OpenTracing teamed up with OpenCensus to create OpenTelemetry. Tracing support is already stable.

Describe the solution you'd like
Replace OpenTracing with OpenTelemetry tracing. Alternatively, add OpenTelemetry to keep backwards compatibility.

[sagikazarmark]

I started looking into this one. A couple notes so far:

• As far as I can tell there is no official SQL instrumentation library for Go (yet) (Funny thing is: the issue about it was closed recently: Opentelemetry for database/sql open-telemetry/opentelemetry-go-contrib#5) Is this something we have to wait for?
• A decision needs to be made about the list of supported exporters for traces (and probably eventually metrics). A logical decision would be using otel's own protocol: they have a reference collector implementation for that and AFAIK Jaeger will support that protocol in the future (or it already does?)

[markphelps]

@sagikazarmark thanks for the update!

I think I'd prefer to add OpenTelemetry as an option for now instead of straight up replacing OpenTracing because of the lack of db/sql support. I personally think db support is a key area for a tracing lib

For your second point, I haven't looked that much into it but it seems that otel already has a few supported exporters for Go: https://opentelemetry.io/docs/instrumentation/go/exporting_data/

It seems they have their own OTEL protocol as well as one for Jaeger and one for Prometheus? Would it make sense to support all 3?

Looking at the current config options for tracing in Flipt, it seems we'd need to change the config struct (and thus the config yaml) a bit to support both OpenTelemetry and OpenTracing while keeping it backwards compatible. We could default to OpenTracing but note that it is deprecated and could be replaced with OpenTelemetry in a future version.

Im thinking the YAML could look something like this?

Existing

tracing:
  jaeger:
    enabled: true

This would still be valid but default to use OpenTracing

New With Otel Support

tracing:
  open_telemery:
    ... some open telemetry specific options
    exporter:
      [otel | jaeger | prometheus]:
        enabled: true  
   open_tracing:
   ... some open tracing specific options
     exporter:
       jaeger:
         enabled: true

With the new approach, you would only be allowed to configure open_telemetry OR open_tracing. Then hopefully if we find support for db/sql tracing in OpenTelemtry we could eventually remove OpenTracing entirely.

What are your thoughts?

[sagikazarmark]

I think I'd prefer to add OpenTelemetry as an option for now instead of straight up replacing OpenTracing

I think I agree, at least until SQL support is added. From that point, it doesn't make much sense to keep both. One could argue that helping the adoption of otel by dropping OT support would be a good thing.

For your second point, I haven't looked that much into it but it seems that otel already has a few supported exporters for Go:

There are several different exporters, otel, jaeger and prometheus might be the most popular ones, but I don't think bundling every single exporter out there is the way to go. Prometheus is an established one, so I'd keep it. Adding otel OOTB also makes sense IMO.

Adding more can easily open up the gate for future requests for adding everyone's favorite exporter. That's exactly why the otel collector exists: keep applications vendor free.

For the rest, we should provide documentation how one can add their own exporters and build a custom version. (That's what I'm trying to do in my own projects).

Im thinking the YAML could look something like this?

I'm still trying to figure out this as well. So far, I had something like this in mind:

telemetry:
    # HTTP server address for telemetry endpoints
    # Potential endpoints: prometheus metrics, pprof debug endpoints, health check endpoint, etc
    address: 127.0.0.1:10000

    tracing:
        # things like sampling rate could go here

    logging:
        level: info

    # Optional level:
    # exporters:

    otlp:
        address: ...
        # ...

    # Optional: having a /metrics endpoint by default is low effort (also: convention over configuration)
    prometheus:
        enabled: true

This way the "old" tracing config could stay in place until OpenTracing is deprecated and removed.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.