feat(tracing): dont trace spans with full request paths as operation name in ExtractFromHTTPRequest

[GeorgeMac]

This updates our automatic span creation from http request method in the tracing kit.

Prior to this change all request paths were being logged in spans as operation names.
This has led to many operation names (and therefore increased cardinality required).

Now we have all operations being reported as request along with the following tags:

• base_path which is an attempt to capture import resource identifiers e.g. (/api/v2/buckets) but not the resource identifier
• handler_name which is the handler which is called to invoke this operation

It also logs the full url path with the key path. This is for a little more context.

My proposed change has increased the number of allocations required. So I thought I would share here for your consideration:

Before:
BenchmarkOpentracing_ExtractFromHTTPRequest-16    	 2000000	      1003 ns/op	     672 B/op	       5 allocs/op



After:
BenchmarkOpentracing_ExtractFromHTTPRequest-16    	 1000000	      1213 ns/op	     816 B/op	      10 allocs/op

Update:

So I have a PR open on httprouter julienschmidt/httprouter#286

This now vendors that change and relies on it to get the router match from the request context (this what my PR allows you to do with httprouter).

This means we don't need to do any complicated deriving of some base path. Instead we tag the spans with the router path as was desired.

e.g. a handler mounted on route /api/v2/buckets/:bucket_id would produce the tag route=/api/v2/buckets/:bucket_id .

goos: darwin
goarch: amd64
pkg: github.com/influxdata/influxdb/kit/tracing
BenchmarkOpentracing_ExtractFromHTTPRequest-16    	 2000000	      1008 ns/op	     688 B/op	       7 allocs/op
PASS
ok  	github.com/influxdata/influxdb/kit/tracing	3.032s

• CHANGELOG.md updated with a link to the PR (not the Issue)
• Well-formatted commit messages
• Rebased/mergeable
• Tests pass
• http/swagger.yml updated (if modified Go structs or API)
• Documentation updated or issue created (provide link to issue/pr)
• Signed CLA (if not already signed)

[jacobmarble]

It is unfortunate that we don't have access to the httprouter path associated with the handler. I just walked through some of the implementation using my debugger, and it appears that there is no way to access that path (I'm talking about /api/v2/foo/:id not the called path).

I also don't like handlerName because it can't be kept consistent between usage. Reflection would be a better way to fill that blank.

What do you think about adjusting ExtractFromHTTPRequest to take routerPath instead of handlerName?

[GeorgeMac]

I tagged you in this @mark-rushakoff to review the approach of doing a hard cut-over to an influxdata fork of httprouter.

[GeorgeMac]

I will come back to this. Do a full replace everywhere means updating flux to use our fork as well.

[jacobmarble]

I thought the go.mod replace was supposed to make import changes unnecessary?

[GeorgeMac]

Yeah as @mark-rushakoff says. This is great until the compat tests run and then it breaks :( but seems like bringing the fork under our banner works.

My latest problem is flux depends on the original version 😂 that is my next challenge. figured it out

[mark-rushakoff]

Works for me.

@jacobmarble using a replacement directive only affects the current module. Projects that import influxdb would still use the julienschmidt/httprouter module, which means they would not be able to compile influxdb due to our httprouter fork becoming intentionally API-incompatible.