Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature #18 add x flow #23

Merged
merged 7 commits into from
Oct 19, 2015
Merged

Feature #18 add x flow #23

merged 7 commits into from
Oct 19, 2015

Conversation

lmineiro
Copy link
Contributor

The implementation generates a random string with 8 to 64 bytes and sets a HTTP header X-Flow-Id that other systems can use to correlate log entries.
The filter accepts some parameters that help control the its behavior, namely, to accept an existing flow id and the length of the generated flow ids
This closes #18

@@ -14,6 +15,10 @@ import (
"github.com/zalando/skipper/skipper"
)

var (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the braces when there is only a single var, but of course fine like this, too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can add some more global errors soon :)

func (this *flowId) MakeFilter(id string, fc skipper.FilterConfig) (skipper.Filter, error) {
var reuseExisting bool
if len(fc) > 0 {
if r, ok := fc[0].(bool); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have bool :(
the doc is not yet in the master, but you can have a look at here:
https://github.com/zalando/skipper/blob/release-cleanup-doc/eskip/doc.go

@mo-gr
Copy link
Contributor

mo-gr commented Oct 16, 2015

Have you considered just having an array of valid chars and then len times pulling a random index out of that one. Seems to me, that this might be faster than crypto-random and hex operations

@lmineiro
Copy link
Contributor Author

Pre-defining an alphabet seems like a good idea. I'm sure it will be faster and, you're right, for this we don't need crypto-randomness.

@lmineiro
Copy link
Contributor Author

$ curl -k -X PUT -d 'value=Path("/flowid") -> flowId() -> "http://localhost:8080"' \
http://127.0.0.1:2379/v2/keys/skipper/routes/testFlowId

$ curl -v http://localhost:9090/flowid
* Hostname was NOT found in DNS cache
*   Trying ::1...
* Connected to localhost (::1) port 9090 (#0)
> GET /flowid HTTP/1.1
> User-Agent: curl/7.38.0
> Host: localhost:9090
> Accept: */*

$ nc -l 8080
GET /flowid HTTP/1.1
Host: localhost:8080
User-Agent: curl/7.38.0
Accept: */*
X-Flow-Id: 4vI2AarIh5hqqsuD
Accept-Encoding: gzip

@aryszka
Copy link
Contributor

aryszka commented Oct 18, 2015

lgtm, very nice

@@ -28,6 +29,7 @@ func Register(registry skipper.FilterRegistry) {
static.Make(),
stripquery.Make(),
&redirect.Redirect{},
flowid.New(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new instead of make? shouldn't we be consistent with naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should. It will be New*(). Refactoring the old filters will be next

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's already in progress to switch to new, see the release-cleanup branch

@danpersa
Copy link
Contributor

lgtm

aryszka added a commit that referenced this pull request Oct 19, 2015
@aryszka aryszka merged commit b1f25e8 into master Oct 19, 2015
@lmineiro lmineiro deleted the feature-#18-add-x-flow-id branch October 19, 2015 14:53
aryszka added a commit that referenced this pull request Mar 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a filter to generate a request id
4 participants