Allow to trace REST requests that match a URI regex by config
Sometimes there are issues that are hard to catch, but which only occur
for a certain REST requests. In this case it is convenient if tracing
for these requests can be enabled by configuration.
It is now possible to configure a regular expression for a request URI
to trigger automatic tracing for matching REST requests.
Change-Id: I264c75259fe1f254fed059bfaa3767af407546f1
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index b27c33c..1601d7a 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4570,6 +4570,16 @@
+
By default, unset (all request types are matched).
+[[tracing.traceid.requestUriPattern]]tracing.<trace-id>.requestUriPattern::
++
+Regular expression to match request URIs for which request tracing
+should be always enabled. Request URIs are only available for REST
+requests. Request URIs never include the '/a' prefix.
++
+May be specified multiple times.
++
+By default, unset (all request URIs are matched).
+
[[tracing.traceid.account]]tracing.<trace-id>.account::
+
Account ID of an account for which request tracing should be always
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 736018a..2128777b 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -317,7 +317,7 @@
try (TraceContext traceContext = enableTracing(req, res)) {
List<IdString> path = splitPath(req);
- RequestInfo requestInfo = createRequestInfo(traceContext, path);
+ RequestInfo requestInfo = createRequestInfo(traceContext, requestUri(req), path);
globals.requestListeners.runEach(l -> l.onRequest(requestInfo));
try (PerThreadCache ignored = PerThreadCache.create()) {
@@ -1424,9 +1424,11 @@
return traceContext;
}
- private RequestInfo createRequestInfo(TraceContext traceContext, List<IdString> path) {
+ private RequestInfo createRequestInfo(
+ TraceContext traceContext, String requestUri, List<IdString> path) {
RequestInfo.Builder requestInfo =
- RequestInfo.builder(RequestInfo.RequestType.REST, globals.currentUser.get(), traceContext);
+ RequestInfo.builder(RequestInfo.RequestType.REST, globals.currentUser.get(), traceContext)
+ .requestUri(requestUri);
if (path.size() < 1) {
return requestInfo.build();
diff --git a/java/com/google/gerrit/server/RequestInfo.java b/java/com/google/gerrit/server/RequestInfo.java
index c86d34d..f5749fc 100644
--- a/java/com/google/gerrit/server/RequestInfo.java
+++ b/java/com/google/gerrit/server/RequestInfo.java
@@ -45,6 +45,15 @@
*/
public abstract String requestType();
+ /**
+ * Request URI.
+ *
+ * <p>Only set if request type is {@link RequestType#REST}.
+ *
+ * <p>Never includes the "/a" prefix.
+ */
+ public abstract Optional<String> requestUri();
+
/** The user that has sent the request. */
public abstract CurrentUser callingUser();
@@ -74,6 +83,8 @@
return requestType(requestType.name());
}
+ public abstract Builder requestUri(String requestUri);
+
public abstract Builder callingUser(CurrentUser callingUser);
public abstract Builder traceContext(TraceContext traceContext);
diff --git a/java/com/google/gerrit/server/TraceRequestListener.java b/java/com/google/gerrit/server/TraceRequestListener.java
index 8651915..773f712 100644
--- a/java/com/google/gerrit/server/TraceRequestListener.java
+++ b/java/com/google/gerrit/server/TraceRequestListener.java
@@ -68,6 +68,7 @@
TraceConfig.Builder traceConfig = TraceConfig.builder();
traceConfig.traceId(traceId);
traceConfig.requestTypes(parseRequestTypes(traceId));
+ traceConfig.requestUriPatterns(parseRequestUriPatterns(traceId));
traceConfig.accountIds(parseAccounts(traceId));
traceConfig.projectPatterns(parseProjectPatterns(traceId));
traceConfigs.add(traceConfig.build());
@@ -83,6 +84,11 @@
return ImmutableSet.copyOf(cfg.getStringList("tracing", traceId, "requestType"));
}
+ private ImmutableSet<Pattern> parseRequestUriPatterns(String traceId)
+ throws ConfigInvalidException {
+ return parsePatterns(traceId, "requestUriPattern");
+ }
+
private ImmutableSet<Account.Id> parseAccounts(String traceId) throws ConfigInvalidException {
ImmutableSet.Builder<Account.Id> accountIds = ImmutableSet.builder();
String[] accounts = cfg.getStringList("tracing", traceId, "account");
@@ -100,19 +106,24 @@
}
private ImmutableSet<Pattern> parseProjectPatterns(String traceId) throws ConfigInvalidException {
- ImmutableSet.Builder<Pattern> projectPatterns = ImmutableSet.builder();
- String[] projectPatternRegExs = cfg.getStringList("tracing", traceId, "projectPattern");
- for (String projectPatternRegEx : projectPatternRegExs) {
+ return parsePatterns(traceId, "projectPattern");
+ }
+
+ private ImmutableSet<Pattern> parsePatterns(String traceId, String name)
+ throws ConfigInvalidException {
+ ImmutableSet.Builder<Pattern> patterns = ImmutableSet.builder();
+ String[] patternRegExs = cfg.getStringList("tracing", traceId, name);
+ for (String patternRegEx : patternRegExs) {
try {
- projectPatterns.add(Pattern.compile(projectPatternRegEx));
+ patterns.add(Pattern.compile(patternRegEx));
} catch (PatternSyntaxException e) {
throw new ConfigInvalidException(
String.format(
- "Invalid tracing config ('tracing.%s.projectPattern = %s'): %s",
- traceId, projectPatternRegEx, e.getMessage()));
+ "Invalid tracing config ('tracing.%s.%s = %s'): %s",
+ traceId, name, patternRegEx, e.getMessage()));
}
}
- return projectPatterns.build();
+ return patterns.build();
}
@AutoValue
@@ -123,6 +134,9 @@
/** request types that should be traced */
abstract ImmutableSet<String> requestTypes();
+ /** pattern matching request URIs */
+ abstract ImmutableSet<Pattern> requestUriPatterns();
+
/** accounts IDs matching calling user */
abstract ImmutableSet<Account.Id> accountIds();
@@ -148,6 +162,20 @@
return false;
}
+ // If in the trace config request URI patterns are set and none of them matches, then the
+ // request is not matched.
+ if (!requestUriPatterns().isEmpty()) {
+ if (!requestInfo.requestUri().isPresent()) {
+ // The request has no request URI, hence it cannot match a request URI pattern.
+ return false;
+ }
+
+ if (requestUriPatterns().stream()
+ .noneMatch(p -> p.matcher(requestInfo.requestUri().get()).matches())) {
+ return false;
+ }
+ }
+
// If in the trace config accounts are set and none of them matches, then the request is not
// matched.
if (!accountIds().isEmpty()) {
@@ -176,8 +204,9 @@
}
}
- // For any match criteria (request type, account, project pattern) that was specified in the
- // trace config, at least one of the configured value matched the request.
+ // For any match criteria (request type, request URI pattern, account, project pattern) that
+ // was specified in the trace config, at least one of the configured value matched the
+ // request.
return true;
}
@@ -187,6 +216,8 @@
abstract Builder requestTypes(ImmutableSet<String> requestTypes);
+ abstract Builder requestUriPatterns(ImmutableSet<Pattern> requestUriPatterns);
+
abstract Builder accountIds(ImmutableSet<Account.Id> accountIds);
abstract Builder projectPatterns(ImmutableSet<Pattern> projectPatterns);
diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
index 8f36191..3a01517 100644
--- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
@@ -542,6 +542,43 @@
assertThat(projectCreationListener.tags.get("project")).containsExactly("new24");
}
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/projects/.*")
+ public void traceRequestUri() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new23");
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(projectCreationListener.traceId).isEqualTo("issue123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new23");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/projects/.*/foo")
+ public void traceRequestUriNoMatch() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new23");
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(projectCreationListener.traceId).isNull();
+ assertThat(projectCreationListener.isLoggingForced).isFalse();
+
+ // The logging tag with the project name is also set if tracing is off.
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new23");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "][")
+ public void traceRequestUriInvalidRegEx() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new24");
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(projectCreationListener.traceId).isNull();
+ assertThat(projectCreationListener.isLoggingForced).isFalse();
+
+ // The logging tag with the project name is also set if tracing is off.
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new24");
+ }
+
private void assertForceLogging(boolean expected) {
assertThat(LoggingContext.getInstance().shouldForceLogging(null, null, false))
.isEqualTo(expected);
