SB-10000: Adding app- and pipeline-ids to server

[joemadeus]

Helix jira ticket SB-10000: https://jira.nyt.net/browse/SB-10000

Pull Request Checklist

• Ticket number has been referenced
• Description has been provided
• Any necessary document changes have been made
• Testing instructions have been provided
• Unit tests have passed
• go fmt, go vet and golint have been run
• Link to the PR has been added to the associated ticket

Description

Adding two types of IDs to gizmo:

• One type application-specific. It generates a random (v4) UUID and optionally prepends a prefix. An interface has been defined so we can specify other schemes if other use cases arise.
• The other type is for the entire pipeline, and is meant to trace a call from its initial request at some top-level application, all the way through.

To support these, middleware has been added that, in both cases, sets the X-Request-ID header (in the case of application IDs, only if the header is not present) to the request and response, and writes it to the context.

Dependencies were updated as a part of running make test (as far as I can tell.) Happy to revert the go.mod and go.sum files if needs be.

[Antonite]

I think this is fine, we don't want to set the request ID to empty string, better to not set it at all.

[joemadeus]

Agreed, but a) I shouldn't have let this TODO slip into the commit, and b) I think of this code as being important but not critical, and the idea that something that isn't critical blitzing stuff that is annoys me. In this case, this err is here solely because the UUID generator.

If you think this is fine I'll remove the TODO and leave the return.

[oliver-nyt]

I had a quick look at the code and it's not clear to me why this needs to live in gixmo.

1. X-Request-ID is already getting picked up by go-kit, see the go-kit repo for how it can be used
2. for tracing requests through your stack of services you should look at e.g. opencensus tracing instead of implementing your own solution. Also, the motivations for tracing aren't clear from the Jira ticket. There's a lot about the "how" but not so much about the "why" which would help with understanding your needs.
3. general style advice re: tests in e.g. TestPipelineIDHandler - have a look at table-driven tests, that's the more Golang idiomatic way to do this instead of the func variable construct you used.

[joemadeus]

@oliver-nyt thanks! I'll answer inline:

1. X-Request-ID is already getting picked up by go-kit, [see the go-kit repo for how it can be used](https://github.com/go-kit/kit/search?q=ContextKeyRequestXRequestID&unscoped_q=ContextKeyRequestXRequestID)

Thanks! That'd definitely be better than the one-off context stuffing in my handler. I assume that'll have to be called after the ID handler runs.

2. for tracing requests through your stack of services you should look at e.g. opencensus tracing instead of implementing your own solution. Also, the motivations for tracing aren't clear from the Jira ticket. There's a lot about the "how" but not so much about the "why" which would help with understanding your needs.

For more info, take a look at the parent ticket for SB-10000: https://jira.nyt.net/browse/SB-9987 As for OpenCensus (and OpenTracing), I looked at CloudTrace (CT) as a way to correlate all the reqs across all our services and it only seemed to want to log the right values (specifically, the trace ID) when CT actually sampled a req for tracing. (In other platforms -- Solarwinds', for instance -- you can use a trace ID whether or not tracing actually sampled the call, but not here.) I wasn't aware that Kit had the OC/OT stuff embedded. I'll take a look.

3. general style advice re: tests in e.g. `TestPipelineIDHandler` - have a look at table-driven tests, that's the more Golang idiomatic way to do this instead of the func variable construct you used.

Fixed!

[jonsabados]

for table driven test stuff each test case should be in a t.run, ie:

func TestPipelineIDHandler(t *testing.T) {
   tests := []struct {
   	desc, prev, next, expected string
   }{
   	{"new", "", "roger", "roger"},
   	{"existing", "roger", "roderick", "roger|roderick"},
   	{"chained", "roger|roderick", "brian", "roger|roderick|brian"},
   }

   for _, test := range tests {
   	t.Run(test.desc, func(t *testing.T) {
   		r, err := http.NewRequest("GET", "", nil)
   		...
   	})
   }
}

[joemadeus]

thanks! fixed.

[joemadeus]

@oliver-nyt , @jonsabados & I did some research on your OpenCensus (and OpenTelemetry) suggestion, and it looks like the code needed to do this is on a wish list, not in a library: census-instrumentation/opencensus-go#950 At Solarwinds (prev. employer) we could create a trace ID whether or not the trace was actually sampled by a collector, and that trace ID would be logged to every apps' logs; I could not find a way to do that with CloudTrace or with OC/OT.

But we may not even want to: we have a couple of fan-out steps in our pipeline and would like to differentiate between requests in the target of the fan-out. We won't be able to if we're using a CT/OC/OT trace ID.

Pls. let me know if you have other thoughts on this!

[oliver-nyt]

I still think this shouldn't be in Gizmo as it's not needed/used by Gizmo itself.
If you need custom code for you to trace logs across your several services then I think this is fine to live in your respective repo.

[joemadeus]

Moving this change to another repo