Support request tracing for REST calls by setting a header in the request

At the moment request tracing for REST calls is enabled by setting the
'trace' or 'trace=<trace-id>' request parameter. This is good for manual
use since adding the request parameter to the URL is very easy and can
be done quickly.

For providing copy-pastable examples for how to do tracing this is not
ideal, since it depends on the concrete URL how the request parameter
would need to be set, e.g. it can be that '?trace' or '&trace' needs to
be appended depending on whether the URL already contains request
parameters or not.

With this change we now support enabling request tracing for REST calls
also by setting a 'X-Gerrit-Trace' header in the request. For manual use
this is less easy but it makes providing copy-pastable examples for how
to do tracing easier as one can now do:

  curl -D /tmp/gerrit -H X-Gerrit-Trace URL
  grep X-Gerrit-Trace /tmp/gerrit

Change-Id: I793ca9fff83ef23f5720390931599a9a85e868c7
Signed-off-by: Edwin Kempin <ekempin@google.com>

Documentation/rest-api.txt

@@ -210,6 +210,15 @@ generated.
   GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/suggest_reviewers?trace&q=J
 ----
 
+Alternatively request tracing can also be enabled by setting the
+`X-Gerrit-Trace` header:
+
+.Example Request
+----
+  GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/suggest_reviewers?q=J
+  X-Gerrit-Trace: issue/123
+----
+
 Enabling tracing results in additional logs with debug information that
 are written to the `error_log`. All logs that correspond to the traced
 request are associated with the trace ID. The trace ID is returned with

Documentation/user-request-tracing.txt

@@ -11,10 +11,10 @@ How tracing is enabled and how the trace ID is returned depends on the
 request type:
 
 * REST API: For REST calls tracing can be enabled by setting the
-  `trace` request parameter, the trace ID is returned as
+  `trace` request parameter or the `X-Gerrit-Trace` header, the trace
-  `X-Gerrit-Trace` header. More information about this can be found in
+  ID is returned as `X-Gerrit-Trace` header. More information about
-  the link:rest-api.html#tracing[Request Tracing] section of the
+  this can be found in the link:rest-api.html#tracing[Request Tracing]
-  link:rest-api.html[REST API documentation].
+  section of the link:rest-api.html[REST API documentation].
 * SSH API: For SSH calls tracing can be enabled by setting the
   `--trace` option. More information about this can be found in
   the link:cmd-index.html#trace[Trace] section of the

java/com/google/gerrit/acceptance/HttpResponse.java

@@ -14,13 +14,16 @@
 
 package com.google.gerrit.acceptance;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.Reader;
 import java.nio.ByteBuffer;
+import java.util.Arrays;
 import org.apache.http.Header;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
@@ -61,6 +64,13 @@ public class HttpResponse {
     return hdr != null ? hdr.getValue() : null;
   }
 
+  public ImmutableList<String> getHeaders(String name) {
+    return Arrays.asList(response.getHeaders(name))
+        .stream()
+        .map(Header::getValue)
+        .collect(toImmutableList());
+  }
+
   public boolean hasContent() {
     Preconditions.checkNotNull(response, "Response is not initialized.");
     return response.getEntity() != null;

java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@@ -110,6 +110,7 @@ import com.google.gerrit.server.audit.ExtendedHttpAuditEvent;
 import com.google.gerrit.server.cache.PerThreadCache;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.LockFailureException;
+import com.google.gerrit.server.logging.RequestId;
 import com.google.gerrit.server.logging.TraceContext;
 import com.google.gerrit.server.permissions.GlobalPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
@@ -1322,11 +1323,42 @@ public class RestApiServlet extends HttpServlet {
   }
 
   private TraceContext enableTracing(HttpServletRequest req, HttpServletResponse res) {
-    String traceValue = req.getParameter(ParameterParser.TRACE_PARAMETER);
+    // There are 2 ways to enable tracing for REST calls:
-    return TraceContext.newTrace(
+    // 1. by using the 'trace' or 'trace=<trace-id>' request parameter
-        traceValue != null,
+    // 2. by setting the 'X-Gerrit-Trace:' or 'X-Gerrit-Trace:<trace-id>' header
-        traceValue,
+    String traceValueFromHeader = req.getHeader(X_GERRIT_TRACE);
-        (tagName, traceId) -> res.setHeader(X_GERRIT_TRACE, traceId.toString()));
+    String traceValueFromRequestParam = req.getParameter(ParameterParser.TRACE_PARAMETER);
+    boolean doTrace = traceValueFromHeader != null || traceValueFromRequestParam != null;
+
+    // Check whether no trace ID, one trace ID or 2 different trace IDs have been specified.
+    String traceId1;
+    String traceId2;
+    if (!Strings.isNullOrEmpty(traceValueFromHeader)) {
+      traceId1 = traceValueFromHeader;
+      if (!Strings.isNullOrEmpty(traceValueFromRequestParam)
+          && !traceValueFromHeader.equals(traceValueFromRequestParam)) {
+        traceId2 = traceValueFromRequestParam;
+      } else {
+        traceId2 = null;
+      }
+    } else {
+      traceId1 = Strings.emptyToNull(traceValueFromRequestParam);
+      traceId2 = null;
+    }
+
+    // Use the first trace ID to start tracing. If this trace ID is null, a trace ID will be
+    // generated.
+    TraceContext traceContext =
+        TraceContext.newTrace(
+            doTrace,
+            traceId1,
+            (tagName, traceId) -> res.setHeader(X_GERRIT_TRACE, traceId.toString()));
+    // If a second trace ID was specified, add a tag for it as well.
+    if (traceId2 != null) {
+      traceContext.addTag(RequestId.Type.TRACE_ID, traceId2);
+      res.addHeader(X_GERRIT_TRACE, traceId2);
+    }
+    return traceContext;
   }
 
   private boolean isDelete(HttpServletRequest req) {

javatests/com/google/gerrit/acceptance/rest/TraceIT.java

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.apache.http.HttpStatus.SC_CREATED;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -36,6 +37,7 @@ import com.google.gerrit.server.validators.ProjectCreationValidationListener;
 import com.google.gerrit.server.validators.ValidationException;
 import com.google.inject.Inject;
 import java.util.List;
+import org.apache.http.message.BasicHeader;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -75,7 +77,7 @@ public class TraceIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void restCallWithTrace() throws Exception {
+  public void restCallWithTraceRequestParam() throws Exception {
     RestResponse response =
         adminRestSession.put("/projects/new2?" + ParameterParser.TRACE_PARAMETER);
     assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
@@ -85,7 +87,7 @@ public class TraceIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void restCallWithTraceAndProvidedTraceId() throws Exception {
+  public void restCallWithTraceRequestParamAndProvidedTraceId() throws Exception {
     RestResponse response =
         adminRestSession.put("/projects/new3?" + ParameterParser.TRACE_PARAMETER + "=issue/123");
     assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
@@ -94,6 +96,70 @@ public class TraceIT extends AbstractDaemonTest {
     assertThat(projectCreationListener.isLoggingForced).isTrue();
   }
 
+  @Test
+  public void restCallWithTraceHeader() throws Exception {
+    RestResponse response =
+        adminRestSession.putWithHeader(
+            "/projects/new4", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, null));
+    assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+    assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull();
+    assertThat(projectCreationListener.traceId).isNotNull();
+    assertThat(projectCreationListener.isLoggingForced).isTrue();
+  }
+
+  @Test
+  public void restCallWithTraceHeaderAndProvidedTraceId() throws Exception {
+    RestResponse response =
+        adminRestSession.putWithHeader(
+            "/projects/new5", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/123"));
+    assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+    assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+    assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+    assertThat(projectCreationListener.isLoggingForced).isTrue();
+  }
+
+  @Test
+  public void restCallWithTraceRequestParamAndTraceHeader() throws Exception {
+    // trace ID only specified by trace header
+    RestResponse response =
+        adminRestSession.putWithHeader(
+            "/projects/new6?trace", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/123"));
+    assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+    assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+    assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+    assertThat(projectCreationListener.isLoggingForced).isTrue();
+
+    // trace ID only specified by trace request parameter
+    response =
+        adminRestSession.putWithHeader(
+            "/projects/new7?trace=issue/123", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, null));
+    assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+    assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+    assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+    assertThat(projectCreationListener.isLoggingForced).isTrue();
+
+    // same trace ID specified by trace header and trace request parameter
+    response =
+        adminRestSession.putWithHeader(
+            "/projects/new8?trace=issue/123",
+            new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/123"));
+    assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+    assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+    assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+    assertThat(projectCreationListener.isLoggingForced).isTrue();
+
+    // different trace IDs specified by trace header and trace request parameter
+    response =
+        adminRestSession.putWithHeader(
+            "/projects/new9?trace=issue/123",
+            new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/456"));
+    assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+    assertThat(response.getHeaders(RestApiServlet.X_GERRIT_TRACE))
+        .containsExactly("issue/123", "issue/456");
+    assertThat(projectCreationListener.traceIds).containsExactly("issue/123", "issue/456");
+    assertThat(projectCreationListener.isLoggingForced).isTrue();
+  }
+
   @Test
   public void pushWithoutTrace() throws Exception {
     PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
@@ -155,12 +221,14 @@ public class TraceIT extends AbstractDaemonTest {
   private static class TraceValidatingProjectCreationValidationListener
       implements ProjectCreationValidationListener {
     String traceId;
+    ImmutableSet<String> traceIds;
     Boolean isLoggingForced;
 
     @Override
     public void validateNewProject(CreateProjectArgs args) throws ValidationException {
       this.traceId =
           Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null);
+      this.traceIds = LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID");
       this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false);
     }
   }