Support passing X-Request-ID to Druid #68

DennisMcWherter · 2016-10-14T19:27:47Z

A note from offline discussion, HTTP headers are case insensitive. See RFC 7230 for more information.

archolewa · 2016-10-14T19:48:53Z

fili-core/src/main/java/com/yahoo/bard/webservice/logging/RequestLog.java

@@ -92,6 +94,7 @@ private RequestLog(RequestLog rl) {
        times = new LinkedHashMap<>(rl.times);
        threadIds = new LinkedHashSet<>(rl.threadIds);
        MDC.put(ID_KEY, logId);
+        idPrefix = "";


This should be set to the idPrefix on the passed-in RequestLog.

archolewa · 2016-10-14T19:51:43Z

fili-core/src/main/java/com/yahoo/bard/webservice/logging/RequestLog.java

+        if (current.info == null) {
+            current.init();
+        }
+        current.idPrefix = (idPrefix == null) ? "" : idPrefix;


This isn't enough. If you look at line 157, you'll see that we are putting the id into MDC. That MDC is what allows us to put the id into every log line generated by a request.

As a result, if we want the prefix to also show up in every log line (we do), then we need to make sure the logId and the idPrefix are added to MDC.

So at the minimum, in the last line in this method, we should add the following:

MDC.put(ID_KEY, getId())

Or manually do the string concatentation if calling the getter in the setter gives you heebie-jeebies.

Although in all honesty, I feel like that opens the door to subtle bugs, if init gets called later and overwrites MDC with the prefix.

Instead, I think that setIdPrefix should prepend the id to logId:

current.logId = idPrefix + current.logId; MDC.put(ID_KEY, current.logId);

That way, if init gets called again at some other random point, we don't have to worry about the prefix being dropped.

archolewa · 2016-10-14T19:55:18Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+    @Override
+    Class<?>[] getResourceClasses() {
+        [ DataServlet.class
+        , BardLoggingFilter.class ]


I would put this on one line. We don't generally wrap unless we have to.

archolewa · 2016-10-14T19:55:40Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+    Map<String, List<String>> getQueryParams() {
+        [
+                "metrics": ["width","depth"],
+                "dateTime": ["2014-06-02%2F2014-06-30"],


The date doesn't have to be URL-encoded.

/NotABlocker

copy/pasta'd :) can unencode if we like

archolewa · 2016-10-14T19:56:05Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+
+    @Override
+    boolean compareResult(String result, String expectedResult, JsonSortStrategy sortStrategy = JsonSortStrategy.SORT_MAPS) {
+        if (!testedDruidQuery) {


What is this and why is it necessary?

the underlying BaseSpec runs 2 tests... One of which we don't want to perform this validation on. Similarly, both tests rely on this compareResult method, so we return true the second time it's run 🤔

I see. We use it compare both the API and the Druid response. How about, instead of using a boolean, we only do the comparison if the expected result contains the context field?

The API response doesn't contain any context mapping.

This also ensures that the test will work even if the tests were run out of order (I don't think Spock makes any guarantee about test order unless you use a special annotation).

archolewa · 2016-10-14T19:59:50Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+    }
+
+    @Override
+    Response makeAbstractRequest(Closure queryParams=this.&getQueryParams) {


I would appreciate a comment indicating that the only difference between this version of makeAbstractRequest and the parent version is that we are adding the X-Request-ID to the header. Otherwise, the two implementations are so similar that people may think this implementation is unnecessary, delete it, and then wonder why the test started failing.

Though a better approach may be to add a hook to makeAbstractRequest. Basically, add a method getAdditionalHeaders that returns headers to add to the request before making the call. We can then have BaseDataServletComponentSpec::makeAbstractRequest add the headers from getAdditionalHeaders to the request just before making the call.

I like the latter solution!

archolewa · 2016-10-14T20:00:51Z

One other note: If you could remember to add PR labels when you open the PR (Reviewable and Need 2 Reviews in particular), it would be much appreciated. The labels make it a lot easier for us to determine the sttaus of the various PR's at a glance.

Thanks for the PR!

archolewa

👍 Assuming you address my small remaining comment.

archolewa · 2016-10-14T20:33:14Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+
+class DruidQueryIdSpec extends BaseDataServletComponentSpec {
+    def prefixId = "abcdef"
+    static def testedDruidQuery = false


This field is no longer necessary, and can be removed.

cdeszaq

Changelog entry needed, as well as a bunch of in-line thoughts.

cdeszaq · 2016-10-17T13:53:22Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/Utils.java

@@ -199,13 +199,13 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx
     * @param fieldName The name of the node to be emitted.


Javadoc update too?

cdeszaq · 2016-10-17T13:58:50Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/BardLoggingFilter.java

@@ -83,6 +84,7 @@
    @Override
    public void filter(ContainerRequestContext request) throws IOException {

+        RequestLog.addIdPrefix(request.getHeaders().getFirst(X_REQUEST_ID_HEADER));


We shouldn't just accept everything that comes in on this header. We should strip this down to some set of allowed characters, and also truncate down to a max length.

Applies to the other places we grab it as well.

What are allowed chars for this header? Are we going to use something like heroku's definition? https://devcenter.heroku.com/articles/http-request-id

I'll go with that for now.

Yeah, that'll work, though it would be good to add periods and underscores (. and _)

And I would say that min-length doesn't matter, and 200 chars for max length seems sane. Stripping any unallowed character (and then truncating) is probably the most permissive approach too

I don't think trying to be "smart" about this value is beneficial since you're going to be looking for some sort of exact match on this string. It likely won't be useful if we try to fix it up.

cdeszaq · 2016-10-17T14:16:31Z

fili-core/src/main/java/com/yahoo/bard/webservice/logging/RequestLog.java

+        RequestLog current = RLOG.get();
+        String newId = idPrefix + getId();
+        current.logId = newId;
+        MDC.put(ID_KEY, newId);


While this will update the current.logId field (and the MDC value), I don't think it will update the UUID that is part of the RequestLog payload. I think that the LogInfo.uuid field (on the object stored in current.info) needs to be updated as well in order for that to happen.

I think the reason this was missed is because we're holding that UUID in 2 places current.logId (we read from that all over), and in the LogInfo.uuid field (current.info). It would be great if we could get rid of the current.logId field and only use the value from the LogInfo object. This refactor isn't required for this PR, but it would be great if we could pull it in sooner, rather than later.

As part of the change around where the logId is accessed from, we should update getId to pull from the single source of truth as well.

Also, since this is the closest I can get to it, it would be cool (if we do this refactoring) to move the MDC.put call in the init method up much higher, so that there's less of a gap between getting a UUID and setting it into MCD.

cdeszaq · 2016-10-17T14:17:14Z

fili-core/src/main/java/com/yahoo/bard/webservice/logging/RequestLog.java

@@ -401,6 +401,18 @@ public static String getId() {
    }

    /**
+     * Set id prefix.


Since it's no longer setting, but is instead appending (so subsequent calls will keep appending), this JavaDoc should make that more clear.

cdeszaq · 2016-10-17T14:19:37Z

.../src/test/groovy/com/yahoo/bard/webservice/web/endpoints/BaseDataServletComponentSpec.groovy

@@ -71,6 +72,10 @@ abstract class BaseDataServletComponentSpec extends Specification {
        populatePhysicalTableAvailability()
    }

+    MultivaluedHashMap<String, String> getAdditionalHeaders() {


It's not clear what these headers are for without tracing how this is used. Perhaps we should expand the method name to something like getAdditionalApiRequestHeaders? A JavaDoc would also help, but not critical if the method name is clearer.

cdeszaq · 2016-10-17T14:24:12Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+import javax.ws.rs.core.MultivaluedHashMap
+
+
+class DruidQueryIdSpec extends BaseDataServletComponentSpec {


It would be good to have this name reflect something about looking for the request ID header to be connected to the druid query id. Perhaps something like RequestIdPrefixesDruidQueryIdSpec?

cdeszaq · 2016-10-17T14:28:10Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+        JsonSlurper slurper = new JsonSlurper(sortStrategy)
+        def parsedJson = slurper.parseText(result)
+        if (parsedJson.context != null) {
+            testedDruidQuery = true


This variable isn't a thing I don't think...

cdeszaq · 2016-10-17T14:29:55Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DruidQueryIdSpec.groovy

+        if (parsedJson.context != null) {
+            testedDruidQuery = true
+            return parsedJson.context.queryId.startsWith(prefixId)
+        }


I think this all can be simplified using Groovy's safe navigation operator:

def parsedQuery = new JsonSlurper(sortStrategy).parseText(result) assert parsedQuery?.context?.queryId?.startsWith(prefixId)

This didn't actually work. This fails in the case where we want this function to act as a noop.

Ahh, I see. you can't just assert in all cases, since not all cases have that condition being true. Makes sense.

cdeszaq

Looking quite good, just need a test for the rules around the request ID and we'll be good for this one.

(That test can be a Spec just against the BardLoggingFilter, and Groovy can call private methods, so it's easy to test this method.)

cdeszaq · 2016-10-17T15:37:34Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/BardLoggingFilter.java

+     *
+     * @param requestId  Request id to add as queryId prefix to druid
+     */
+    private void appendRequestId(String requestId) {


It would be good to have a test for this, validating the rules.

cdeszaq · 2016-10-17T16:45:59Z

👍

archolewa · 2016-10-17T19:02:31Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/BardLoggingFilter.java

+     * @param requestId  Request id to add as queryId prefix to druid
+     */
+    private void appendRequestId(String requestId) {
+        if (isValidRequestId(requestId)) {


I don't understand this line. We are ignoring a requestId if it is a valid request id? Wouldn't we want to ignore the request id if it isn't valid?

Good catch. I think the behavior is correct (yay testing) the name is just wrong. @DeathByTape, could you open a quick PR to correct this?

archolewa · 2016-10-17T19:04:01Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/filters/BardLoggingFilter.java

+     */
+    private boolean isValidRequestId(String requestId) {
+        return requestId == null || requestId.isEmpty() || requestId.length() > 200
+               || !VALID_REQUEST_ID.matcher(requestId).matches();


It looks like this method is actually returning the opposite of what it claims. It's returning true if the requestId is not valid, and false if it is valid.

archolewa requested changes Oct 14, 2016

View reviewed changes

archolewa added NEED CHANGES NEED 2 REVIEWS REVIEWABLE labels Oct 14, 2016

DennisMcWherter force-pushed the xRequestId branch 2 times, most recently from efae917 to 4667c29 Compare October 14, 2016 20:32

archolewa approved these changes Oct 14, 2016

View reviewed changes

archolewa added NEED 1 REVIEW NEED CHANGES and removed NEED CHANGES NEED 2 REVIEWS labels Oct 14, 2016

DennisMcWherter force-pushed the xRequestId branch from 4667c29 to 6ed9cd4 Compare October 14, 2016 20:34

cdeszaq requested changes Oct 17, 2016

View reviewed changes

DennisMcWherter force-pushed the xRequestId branch 2 times, most recently from 4e146bd to 9eb90e4 Compare October 17, 2016 15:33

cdeszaq requested changes Oct 17, 2016

View reviewed changes

Added support for x-request-id header to prefix druid query id.

58eaa42

DennisMcWherter force-pushed the xRequestId branch from 3109113 to 58eaa42 Compare October 17, 2016 16:34

cdeszaq approved these changes Oct 17, 2016

View reviewed changes

cdeszaq added MERGEABLE and removed NEED 1 REVIEW NEED CHANGES REVIEWABLE labels Oct 17, 2016

cdeszaq merged commit 2c04321 into yahoo:master Oct 17, 2016

DennisMcWherter deleted the xRequestId branch October 17, 2016 17:44

archolewa reviewed Oct 17, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing X-Request-ID to Druid #68

Support passing X-Request-ID to Druid #68

DennisMcWherter commented Oct 14, 2016 •

edited

archolewa Oct 14, 2016

archolewa Oct 14, 2016 •

edited

archolewa Oct 14, 2016

archolewa Oct 14, 2016

DennisMcWherter Oct 14, 2016

archolewa Oct 14, 2016

DennisMcWherter Oct 14, 2016

archolewa Oct 14, 2016 •

edited

archolewa Oct 14, 2016

DennisMcWherter Oct 14, 2016

archolewa commented Oct 14, 2016 •

edited

archolewa left a comment

archolewa Oct 14, 2016

cdeszaq left a comment

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

DennisMcWherter Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

DennisMcWherter Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq Oct 17, 2016

DennisMcWherter Oct 17, 2016

cdeszaq Oct 17, 2016

cdeszaq left a comment

cdeszaq Oct 17, 2016

cdeszaq commented Oct 17, 2016

archolewa Oct 17, 2016

cdeszaq Oct 17, 2016

archolewa Oct 17, 2016

		@@ -199,13 +199,13 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx
		* @param fieldName The name of the node to be emitted.

		import javax.ws.rs.core.MultivaluedHashMap


		class DruidQueryIdSpec extends BaseDataServletComponentSpec {

Support passing X-Request-ID to Druid #68

Support passing X-Request-ID to Druid #68

Conversation

DennisMcWherter commented Oct 14, 2016 • edited

Choose a reason for hiding this comment

archolewa Oct 14, 2016 • edited

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

Choose a reason for hiding this comment

archolewa Oct 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archolewa commented Oct 14, 2016 • edited

archolewa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq 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

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

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

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DennisMcWherter commented Oct 14, 2016 •

edited

archolewa Oct 14, 2016 •

edited

archolewa Oct 14, 2016 •

edited

archolewa commented Oct 14, 2016 •

edited