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
Add timestamp header for New Relic request queue #1672
Conversation
@@ -20,19 +20,19 @@ object CurrentIstioPath { | |||
} | |||
|
|||
/** | |||
* Returns the target service requested based on the supplied [[Path]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like Scalariform modified this and I committed it by mistake. will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about request header vs response header and how that end-to-end test passes.
|
||
val downstream = Downstream.mk("dog") { | ||
req => | ||
req.headerMap.keys must contain ("x-request-start") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also check that the value can be parsed as a long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had wanted to test using Time.withCurrentTimeAt
but that wasn't practical in the e2e test; that seems like a good compromise.
|
||
val DefaultHeader: String = "X-Request-Start" | ||
|
||
case class Param(header: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this param should contain an Option[String]
so that this filter can be disabled. (and it should be disabled by default)
*/ | ||
object TimestampHeaderFilter { | ||
|
||
val DefaultHeader: String = "X-Request-Start" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this since this should be disabled by default
next: ServiceFactory[Request, Response] | ||
): ServiceFactory[Request, Response] = { | ||
val Param(header) = param | ||
filter(header) andThen next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to avoid infix notation
} | ||
|
||
def filter(header: String): Filter[Request, Response, Request, Response] = | ||
(req: Request, svc: Service[Request, Response]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer Filter.mk
or new Filter
|
||
val c = upstream(s) | ||
try { | ||
val resp = await(c(req)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to ensure that this request was even routed to the downstream correctly, should assert the status code is ok
val resp = await(c(req)) | ||
} finally { | ||
await(c.close()) | ||
await(s.close()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close the downstream server too
def filter(header: String): Filter[Request, Response, Request, Response] = | ||
(req: Request, svc: Service[Request, Response]) => { | ||
val reqT = Time.now | ||
svc(req).map { rsp => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this puts the header on the response, not the request?
@@ -499,6 +497,82 @@ class HttpEndToEndTest extends FunSuite with Awaits with MustMatchers { | |||
|
|||
} | |||
|
|||
test("timestampHeader adds header") { | |||
var headers: Option[HeaderMap] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might have to be @volatile since it's shared across threads
/** | ||
* Adds a timestamp header to a message. | ||
* | ||
* This is intended to be used for the New Relic request queue's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove lines 10-11
import com.twitter.util.{Future, Time} | ||
|
||
/** | ||
* Adds a timestamp header to a message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/message/request
@adleong I think I've addressed all requested changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⏲ 🚛 🖲
## 1.3.1 2017-10-24 * Kubernetes * Fixed a failure to update routing data after restarting watches (#1674). * Ensured that Kubernetes API watch events earlier than the current state are ignored (#1681). * Added support for Istio Mixer precondition checks (#1606). * Removed spurious error message logging from Consul namer (#1682). * Changed DNS SRV record namer to use system DNS resolver configuration (#1679). * Added `timestampHeader` configuration to support New Relic request queue (#1672).
As discussed in #1664 (comment), the New Relic request queue uses a
X-Request-Start
header with a timestamp to measure latency. I've added a filter that allows Linkerd to add such a header to outbound requests. I've added unit and end-to-end tests for this feature, and added the header configuration to the config docs.