Add unique request id #292

bkmit · 2017-05-12T14:54:33Z

This patch add proxy.header.uniquerequestid configuration option for
specifying header name for adding unique ID to each http request proxied by
fabio.
This header value can be logged in fabio access log and used for request
processing tracking across different applications.

CLAassistant · 2017-05-12T14:54:40Z

All committers have signed the CLA.

magiconair

I think this is a good idea. A couple of comments:

I don't see the code that handles the access logging.
Please update the NOTICES.txt file for each vendored library.

However, unless the unique request id format is fully configurable and pluggable I'd prefer if you just write a function which generates a UUID instead of pulling the library in for that one function.

magiconair · 2017-05-12T17:17:55Z

proxy/http_headers_test.go

+				"X-Forwarded-Proto":   []string{"http"},
+				"X-Forwarded-Port":    []string{"1234"},
+				"X-Real-Ip":           []string{"1.2.3.4"},
+				"X-Unique-Request-Id": []string{""}, // XXX nedd to figure out how to regexp match


You add a func UUID() to the proxy which you can mock.

Made changes you suggest.

Logging can be made by using $header.ConfiguredHeaderName format already present in fabio.

magiconair · 2017-05-12T17:18:09Z

fabio.properties

@@ -353,6 +353,13 @@
 # proxy.header.tls.value =


+# proxy.header.uniquerequesti configures the header for the adding unique request id.


magiconair · 2017-05-12T17:19:01Z

config/config.go

@@ -57,6 +57,7 @@ type Proxy struct {
 	TLSHeader             string
 	TLSHeaderValue        string
 	GZIPContentTypes      *regexp.Regexp
+	UniqueRequestID       string


I think RequestID is enough since they need to be unique for this to make sense.

Shortened option name

magiconair

Can you move the UUID code into uuid/uuid.go and put into the header a copyright notice where you copied this from, please? Also, update the NOTICES.txt file with a reference to that project and the license needs to be compatible (MIT, BSD, ...).

magiconair · 2017-05-12T18:58:36Z

you're also correct with the logging.

magiconair · 2017-05-12T19:40:08Z

Almost there. For testing I suggest you add a UUID function to the proxy similar to the Time function:

// UUID returns a unique id in uuid format.
// If UUID is nil, uuid.NewV4() is used.
UUID func() string

Then create in the uuid package:

func NewV4() string {
    return EncodeHex(NewRandom())
}

Now you can mock this in the test.

magiconair · 2017-05-12T19:42:16Z

One thing I'd like to know though and maybe you can add a benchmark for this is how many UUIDs this approach can generate? Is this using a pseudo random number generator which can generate enough entropy for thousands of req/sec or will this become a limiting factor for the throughput? If yes, then we need to find a better way.

magiconair · 2017-05-12T19:43:32Z

There are users with 20k req/sec (or more) on a single instance. I won't name you in public but you know who you are :)

aaronhurt · 2017-05-12T20:40:23Z

Think I found a winner ... it's not RFC 1422 compliant but it does generate unique IDs and it does it very fast. The other standard UUID packages around github do won't meet the 20k/s request requirement.

Code

package main

import (
	"fmt"
	"testing"

	guuid "github.com/google/uuid"
	muuid "github.com/m4rw3r/uuid"
	fuuid "github.com/rogpeppe/fastuuid"
	suuid "github.com/satori/go.uuid"
)

func BenchmarkV4Google(b *testing.B) {
	for i := 0; i < b.N; i++ {
		guuid.NewRandom()
	}
}

func BenchmarkV4Satori(b *testing.B) {
	for i := 0; i < b.N; i++ {
		suuid.NewV4()
	}
}

func BenchmarkV4m4rw3r(b *testing.B) {
	for i := 0; i < b.N; i++ {
		muuid.V4()
	}
}

func BenchmarkFastUUIDRaw(b *testing.B) {
	g := fuuid.MustNewGenerator()
	for i := 0; i < b.N; i++ {
		g.Next()
	}
}

func BenchmarkFastUUIDFormatted(b *testing.B) {
	g := fuuid.MustNewGenerator()
	for i := 0; i < b.N; i++ {
		_ = fmt.Sprintf("%x", g.Next())
	}
}

Results

ahurt1:uuid-test ahurt$ go test -bench . -benchtime 10s
BenchmarkV4Google-4            	10000000	      1292 ns/op
BenchmarkV4Satori-4            	10000000	      1272 ns/op
BenchmarkV4m4rw3r-4            	10000000	      1285 ns/op
BenchmarkFastUUIDRaw-4         	1000000000	        20.0 ns/op
BenchmarkFastUUIDFormatted-4   	30000000	       505 ns/op
PASS
ok  	github.com/leprechau/uuid-test	80.111s
ahurt1:uuid-test ahurt$

Even with the formatting being done by the relatively heavy fmt package the fastuuid package is the clear winner and would exceed the required performance metric.

magiconair · 2017-05-12T20:45:19Z

@leprechau very cool. Thx for digging. Want to add a fast formatter? That lib is also small enough to be vendored in.

aaronhurt · 2017-05-12T21:19:06Z

@magiconair Done. I put the formatter and test here: https://github.com/leprechau/uuid-test
@bkmit Feel free to use/adopt as needed.

bkmit · 2017-05-12T21:59:59Z

Wow! Great research! I'll definitely use fastest library.

bkmit · 2017-05-16T06:42:28Z

@magiconair Do I need to make some more improvements?

magiconair · 2017-05-16T07:00:04Z

Sorry. Got sidetracked. Looking now.

magiconair

Some small changes but other than that this looks quite good!

Could you add an integration test as well? You can either copy the TestProxyProducesCorrectXffHeader or refactor/rename it slightly so that it tests for more than one header. I'd like to make sure that this doesn't get lost during some refactoring.

magiconair · 2017-05-16T07:02:37Z

fabio.properties

@@ -353,6 +353,13 @@
 # proxy.header.tls.value =


+# proxy.header.requestid configures the header for the adding request id.


proxy.header.requestid configures the header for the adding a unique request id.

magiconair · 2017-05-16T07:11:24Z

proxy/http_headers.go

+// addRequestID Set header with name from `cfg.RequestID` to UUID value
+//
+func addRequestID(r *http.Request, p *HTTPProxy) {
+	if p.Config.RequestID != "" {


Can you add this to the addHeaders() function and move your test to the TestAddHeaders() fn as well, please? This isn't complex enough for a separate fn/test IMO.

if p.Config.RequestID != "" { id := p.UUID if id == nil { id = uuid.NewUUID } r.Header.Set(p.Config.RequestID, id()) }

I have did it in separate function because it need HTTPProxy, which is not available in addHeaders.
Changing addHeaders signature lead to massive change of it unit tests.

I see. Then just inline the couple lines into the HTTP proxy and use the integration test to test it.

magiconair · 2017-05-16T11:04:15Z

Looks good. One last request. Can you split this up into just two commits? One for the vendored in library and one for the feature change? I don't want to squash those two.

magiconair · 2017-05-16T11:26:34Z

Once you're done pls force push the change so that it replaces the current list of commits.

specifying header name for adding unique ID to each http request proxied by fabio. This header value can be logged in fabio access log and used for request processing tracking across different request handling applications.

This patch add proxy.header.requestid configuration option for specifying header name for adding unique ID to each http request proxied by fabio. This header value can be logged in fabio access log and used for request processing tracking across different request handling applications.

magiconair · 2017-05-16T19:05:46Z

merged it.

magiconair · 2017-05-16T19:06:04Z

@bkmit Thanks for your patience :)

bkmit · 2017-05-16T19:11:15Z

@magiconair Many thanks for your patience! This was my first public golang development (micro) project!

magiconair · 2017-05-16T19:13:08Z

For that the quality was quite high! Hats off.

magiconair suggested changes May 12, 2017

View reviewed changes

magiconair changed the title ~~This patch add uniquerequestid configuration option.~~ Add unique request id May 12, 2017

magiconair reviewed May 16, 2017

View reviewed changes

magiconair added the enhancement label May 16, 2017

bkmit added 2 commits May 16, 2017 15:19

Vendor in github.com/rogpeppe/fastuuid for request id generation.

d3ad134

This patch add proxy.header.requestid configuration option for

e476702

specifying header name for adding unique ID to each http request proxied by fabio. This header value can be logged in fabio access log and used for request processing tracking across different request handling applications.

bkmit force-pushed the master branch from fb309ef to e476702 Compare May 16, 2017 12:31

magiconair approved these changes May 16, 2017

View reviewed changes

magiconair closed this May 16, 2017

magiconair added this to the 1.5.0 milestone Oct 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unique request id #292

Add unique request id #292

bkmit commented May 12, 2017

CLAassistant commented May 12, 2017 •

edited

Loading

magiconair left a comment

magiconair May 12, 2017

bkmit May 12, 2017

bkmit May 12, 2017 •

edited

Loading

magiconair May 12, 2017

bkmit May 12, 2017

magiconair May 12, 2017

bkmit May 12, 2017

magiconair left a comment

magiconair commented May 12, 2017

magiconair commented May 12, 2017

magiconair commented May 12, 2017

magiconair commented May 12, 2017

aaronhurt commented May 12, 2017 •

edited

Loading

magiconair commented May 12, 2017

aaronhurt commented May 12, 2017

bkmit commented May 12, 2017

bkmit commented May 16, 2017

magiconair commented May 16, 2017

magiconair left a comment

magiconair May 16, 2017

magiconair May 16, 2017

bkmit May 16, 2017

magiconair May 16, 2017

magiconair commented May 16, 2017

magiconair commented May 16, 2017

magiconair commented May 16, 2017

magiconair commented May 16, 2017

bkmit commented May 16, 2017

magiconair commented May 16, 2017

		@@ -353,6 +353,13 @@
		# proxy.header.tls.value =


		# proxy.header.uniquerequesti configures the header for the adding unique request id.

		@@ -353,6 +353,13 @@
		# proxy.header.tls.value =


		# proxy.header.requestid configures the header for the adding request id.

Add unique request id #292

Add unique request id #292

Conversation

bkmit commented May 12, 2017

CLAassistant commented May 12, 2017 • edited Loading

magiconair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkmit May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair left a comment

Choose a reason for hiding this comment

magiconair commented May 12, 2017

magiconair commented May 12, 2017

magiconair commented May 12, 2017

magiconair commented May 12, 2017

aaronhurt commented May 12, 2017 • edited Loading

magiconair commented May 12, 2017

aaronhurt commented May 12, 2017

bkmit commented May 12, 2017

bkmit commented May 16, 2017

magiconair commented May 16, 2017

magiconair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented May 16, 2017

magiconair commented May 16, 2017

magiconair commented May 16, 2017

magiconair commented May 16, 2017

bkmit commented May 16, 2017

magiconair commented May 16, 2017

CLAassistant commented May 12, 2017 •

edited

Loading

bkmit May 12, 2017 •

edited

Loading

aaronhurt commented May 12, 2017 •

edited

Loading