Added support for sending traces to OpenTelemetry

[oxtoacart]

This PR adds support for sending ops data to an OpenTelemetry endpoint as traces. The sample rate can be configured through the global config, and all ops that are fully sampled by Borda are also fully sampled by OpenTelemetry.

[oxtoacart]

This sample rate matches Borda's current sample rate of 0.01%.

[lincolnmantracer]

I think it's easier to specify the sample rate as an integer, where the ratio is 1/samplerate (this also allows us to re-weight without losing precision both ways)

[oxtoacart]

Sorry, I'm not sure I follow. What's the problem with using decimal rates?

[oxtoacart]

Well, by modeling this after Honeycomb's DeterministicSampler, I ended up switching to Integer rates anyway :)

[myleshorton]

I feel like we might want to make these configurable if they're not already? I worry this could otherwise hammer honeycomb

[oxtoacart]

Will do.

[lincolnmantracer]

This is also the only place in the otel code where we reference borda. Probably worth splitting it out. I can't find where this is defined at a glance, can you link it?

One note - this could muck with our re-weighting of sampled traces.

[oxtoacart]

No longer an issue

[lincolnmantracer]

We need attribute.Key("SampleRate").Int64(1/cfg.SampleRate) (with some type fudging) so that honeycomb re-weights counts.

[lincolnmantracer]

This also appears to be wrong in github.com/getlantern/telemetry. (cc @myleshorton)

[oxtoacart]

I'm handling this in our sampler on a span by span basis.

[lincolnmantracer]

Is there a way to not do this?

[lincolnmantracer]

Oh I guess that would be metrics.iantem.io next.

[lincolnmantracer]

Why is it necessary for this code to be thread safe?

[oxtoacart]

Because the stopper can be called through the public function Stop(), which can theoretically run concurrently with new calls to Configure(). Configure() is called every time our global config changes, so it happens multiple times during the running of the program. Stop() is really only called when the app shuts down, but in theory that could happen concurrently with an ongoing Configure().

[lincolnmantracer]

Gotcha - I understand the model now. I am a bit concerned that otel has global state (otel.SetTracerProvider) and that we don't understand how thread-safe that is (it appears that it is, but that's also not in the contract). Do we already have locked global state in flashlight somewhere? If so we could just put a TracerProvider in there, and it might simplify this code.

[lincolnmantracer]

We could also full rely on otel's thread-safety and do something like

oldTp := otel.GetTracerProvider()
otel.SetTracerProvider(newTp)

if oldTp != nil {
    oldTp.Shutdown()
}

Shutdown appears to call exporter.Shutdown. (At least the documentation says that it does.)

[oxtoacart]

I like this, but sadly GetTracerProvider() returns an interface that doesn't expose Shutdown(). I can cast to the concrete type, but if someone set a different implementation of TracerProvider then we get some weirdness. I think I'll leave as-is.

[lincolnmantracer]

Why would they do that? That's super annoying. What about

interface Shutdowner {
     Shutdown()
}

if shutdowner, ok := oldTp.(Shutdowner); ok {
    shutdowner.Shutdown()
}

[lincolnmantracer]

On the other hand, we SetTracerProvider a few lines above, so it doesn't seem a stretch to try to cast to that type.

[lincolnmantracer]

I'll leave this here: open-telemetry/opentelemetry-go#23

[lincolnmantracer]

Hanging for a minute when a user is trying to close the app (?) seems bad. And the otel exporter should be much faster than that.

[oxtoacart]

It's faster than that under good network conditions, but in a censored environment, potentially going over domain fronting (not yet implemented), it can take a moment. The alternative is to lose potentially valuable telemetry data.

[lincolnmantracer]

What is the rationale for treating these ops differently?

[oxtoacart]

It varies by op, but let's take something like client_started as an example. We may want to use that as a signifier that the client ran at all and use that to derive our user numbers. This even happens only once per startup, so if we're sampling it at a low rate, we'll completely miss a bunch of clients. If we're interesting in looking for clients of a specific app version in a small country, this means that we could completely miss the presence of these if they just didn't make the cut for getting sampled.

[oxtoacart]

More generally, for low frequency ops, sampling at low rates risks missing them completely, so it's desirable to sample some things at higher rates.

[lincolnmantracer]

I'm not seeing what this protects against. If it's just opSampleRates (isn't that defined at startup?) then we could make a copy of the map in the opAwareSampler.

[lincolnmantracer]

Oh, I think I was missing that we reload the global config sometimes. I would release the lock right after we read the sample right.

This is maybe a stupid question, but could we just restart flashlight when it changes instead?

[oxtoacart]

Flashlight is constantly handling client connections, so restarting the whole process is really disruptive.

[lincolnmantracer]

Actually, Configure gets called when we reload the config, which creates a new opAwareSampler, right? So we can actually copy the op sample rates into the struct and not worry about locking here. I think it's worth it since this will be called concurrently.

[oxtoacart]

I like using the copy, I've implemented that.

[oxtoacart]

The CI failures don't seem to be specific to this PR, so I'm going to merge.