OpenTelemetry for launcher

[RebeccaMahany]

Adds traces to a couple locations in launcher, as well as an optional exporter using OTLP over HTTP. These changes work out-of-the-box with the Jaeger all-in-one container documented here. Adds a command-line flag to disable exporting traces.

[directionless]

I love it, but I want to noodle on the function signatures

[directionless]

This is an interesting question... I don't know if we should add spans to kryptoEcMiddleware.Wrap vs wrapping with a span middleware. Thoughts?

[RebeccaMahany]

I thought it was helpful this way because it gives us more specific timing info for the kryptoEcMiddleware, which would've been helpful in the past. We do already effectively wrap the entire localhost handler with the span middleware -- we use otelhttp.NewHandler to wrap all our other handlers. So we have an outer span for the initial request, a child span for the kryptoEcMiddleware, and then a final child span for the request handling (e.g. control server acceleration or whatever we're doing).

[directionless]

That's a good point -- maybe we don't usually want to instrument a span, but this one might be special.

[RebeccaMahany]

I'm not sure I get the original point -- with the exception of the otelhttp wrapper, I'm mostly manually instrumenting spans here. Is that something we don't want?

[directionless]

There were a couple aspects to the original comment, but I'm not sure they're germane.

Foremost, my point is about how if we want to instrument the entire span, we'd be better off doing it in middleware. Though you're correct that otelhttp.NewHandler is already that middleware.

The secondary point is that if we're instrumenting the span with otelhttp.NewHandler, then immediately starting a subspan isn't going to be meaningful. It should be the same timing period. If there are smaller parts we want to trace it makes sense, but this looks like:

trace1{
  trace2{
    stuff
  }
}

So I don't see it adding much

[directionless]

Though I see that you need the span to add in the error. But I wonder if SpanFromContext is a better fit.

Basically, why add the span? Does it help any?

[RebeccaMahany]

In general I like more child spans because they help us narrow down causes of performance issues. E.g. here, if we have a localserver request that we see take a long time, we can use the multiple child spans to identify if it's the krypto middleware that's taking a while, or the inner request to e.g. accelerate the control server interval. Without the child spans, all we have is one span that tells us it takes a certain amount of time for the request to be processed and a response to be returned, which isn't really more info than what we've got in the browser dev console.

[directionless]

Right. But only if there are those inner spans. So why wrap the whole function, vs just the blocks you want? There's an extra span here

[RebeccaMahany]

I suppose I find it a little easier to view? All the spans I care about for troubleshooting one particular code path, grouped into one trace. And I appreciate when tracing/logging is done in a consistent way.

[directionless]

I think we're talking past each other. But we can point at a screen with real data and iterate later

[directionless]

I want to add spans to the generate, but I think we should be doing it at a different level.

[RebeccaMahany]

Where do you have in mind?

[directionless]

Probably the osquery-go SDK, or even thrift.

[RebeccaMahany]

I imagine we'll come back to that idea a little later?

[directionless]

I'm not sure I can follow the exporter, but this seems about right.

I think we can add some sugar around adding errors. trace.AddErrorToCtx(ctx, err) and trace.AddErrorToSpan(span, err)`maybe? But we could kick that forward.

[directionless]

I don't know what this value is -- is the path separator going to vary by platform?

[RebeccaMahany]

It's the closest I got to getting the package name -- e.g. when the caller is ee/localserver/request-controlservice.go, this will be localserver. I can document this better.

[RebeccaMahany]

Added a comment to make this clearer

[directionless]

On windows, are the paths / or \? (I'm never sure, since it's oddly inconsistent. windows paths are, of course \, but sometimes the internal go paths are /)

[RebeccaMahany]

Oh sorry, I didn't realize I'd missed that part of the question -- let me confirm.

[RebeccaMahany]

Forward slash also -- so no, path separator will not vary by platform.

[directionless]

Then it'll need path not filepath and a comment.

[directionless]

I'd say fix the typecasts, and it's a solid base to iterate with

[seejdev]

Seems like a solid starting point! Great job!

[James-Pickett]

awesome work!