teamhephy / router

MIT License
4 stars 10 forks source link

Enable `X-REQUEST-START` header on routers #42

Closed pdomagala closed 4 years ago

pdomagala commented 4 years ago

It would be great if we can enable/disable x-request-start header on specific app.

below discussion from slack:

pd Today at 3:20 PM Is there any way to add X-REQUEST-START header on deis-routers? 34 replies cryptophobia 2 hours ago Is that an nginx header? If it is then we would have to add an extra app configuration annotation. pd 2 hours ago yes, that’s nginx header pd 1 hour ago cryptophobia I’m not sure if I understood correctly, so basically I can add an annotation to app? cryptophobia 26 minutes ago We will need to add code to the router config generator to allow it as an annotation on the application service object. Similar to how this one works https://github.com/teamhephy/router/blob/master/README.md#ssl-enforce GitHubGitHub teamhephy/router Contribute to teamhephy/router development by creating an account on GitHub. cryptophobia 26 minutes ago Why do you need this annotation? Sounds like a special case. Otherwise we would add to all nginx server block configs for all apps. felixbuenemann felixbuenemann 23 minutes ago What is the purpose of X-Request-Start? Do you want to measure queuing latency from the router to your app layer? felixbuenemann 14 minutes ago cryptophobia I just checked, we could add something like proxy_set_header X-Request-Start "t=${msec}"; for all apps, I'm not sure we need an annotation to turn it on and off. felixbuenemann 13 minutes ago Some perf monitoring solutions like newrelic apm can use that header to determine how long the request was queued in the proxy layer before being processed in the app. pd 12 minutes ago exactly, we want to enable those metrics on newrelic cryptophobia 11 minutes ago We can add a global key to enable for all apps on the router annotations so we don't change current behavior felixbuenemann? cryptophobia 11 minutes ago Was not sure how this header works and what it is for so was just making assumptions... felixbuenemann 10 minutes ago The only reason you would want to disable if is if you had another proxy layer like haproxy in front of the deis router, that already injects the header. So not sure we need a flag to turn it on and off. cryptophobia 10 minutes ago Sure, if everyone agrees then I can work on adding to config for all apps. cryptophobia 10 minutes ago Sounds like it's not a header that changes current behavior, mostly used for analytics of latency and apm. pd 9 minutes ago If I can suggest, It’ll be great if we can disable/enable it for apps pd 9 minutes ago because in other infrastructure, we have it set on Haproxy and I think it can broke our metrics felixbuenemann 9 minutes ago pd Do you have haproxy in front of deis router or behind it? pd 8 minutes ago we’ve haproxy in front of deis router felixbuenemann 8 minutes ago If you are running haproxy behind deis router you'd probably just override the header. pd 8 minutes ago ELB -> Haproxy -> Deis-routers felixbuenemann 8 minutes ago Ah OK, then currently your best way would be to add the header on haproxy. felixbuenemann 8 minutes ago Otherwise you ignore queuing time in haproxy. pd 7 minutes ago sure, but in another project, we don’t have haproxy, so the only place where we can enable this header is deis-routers. pd 6 minutes ago give me a second, I’ll take a look if nginx will override that header if it’s already set before cryptophobia 6 minutes ago pd can you open an issue in teamhephy/router repo and reference the above conversation. I'll add it for next release. felixbuenemann 6 minutes ago In that case the normal annotation system should work fine, that can enable/disable globally in the router and also per app. cryptophobia 6 minutes ago Also mention we agree on global enable/disable switch. pd felixbuenemann 6 minutes ago Another approach would be to add the header only if it isn't already present in the request. cryptophobia 5 minutes ago Hmm how would you do the above felixbuenemann? felixbuenemann 5 minutes ago That would be rather using a map in nginx. felixbuenemann 4 minutes ago you would map over $http_x_request_stat and if is has a value pass it though and if not, set it. Then you use the new variable from that map in proxy_set_header. felixbuenemann 4 minutes ago It also mean it's possible to fake that header as a client, so it's not without drawbacks. felixbuenemann 3 minutes ago So probably an on/off toggle is the better solution.

Cryptophobia commented 4 years ago

@pdomagala , I will get this code in for next release. Thank you for opening the issue with the conversation from Slack. As of right now, it sounds like it would be best if it is a global switch and then a switch for each app to enable and disable the header.

The global switch will enable for all apps and be the easiest option for the users to enable this optional header.

The global switch will enable it for all apps by default underneath the hephy-router. If a user wants to disable it for certain apps that are using another loadbalancer, then the user can opt-in to add an annotation to the application service and disable them for each specific app that does not need the header underneath the hephy-router. It requires more configuration per app but this is a very custom setup so the extra configuration is warranted.

I am thinking for the names of the annotations to be:

Component Resource Type Annotation Default Value
deis-router deployment router.deis.io/nginx.requestStartHeader false
routable application service router.deis.io/disableRequestStartHeader N/A*

How does that sounds @felixbuenemann and @pdomagala ?

Will the X-Request-Start header be ever set to anything different from t=${msec}?

pdomagala commented 4 years ago

Sounds great to me! t=${msec} is perfect, NewRelic recommends the same value in their docs.

Thanks for fast response 👍

felixbuenemann commented 4 years ago

I would simplify the name to requestStart or requestStartHeader if the shorter version seems confusing, because the option that toggles X-Request-ID headers is simply called requestIDs.

Cryptophobia commented 4 years ago

Per the discussion above I have made the annotations simpler:

router.deis.io/nginx.requestStartHeader router.deis.io/disableRequestStartHeader

First one is set on global hephy-router annotations, the other is the application annotation to opt-out of the X-Request-Start header for each application if necessary.

Cryptophobia commented 4 years ago

Looks like we will be modifying the PR a bit:

So if appConfig.RequestStartHeader is nil, we use the value from routerConfig.RequestStartHeader, but if it is either true or false it overrides the routerConfig value. - @felixbuenemann

Cryptophobia commented 4 years ago

This can be tested with container cryptophobia/router:git-6016a3d.