Merge changes I264c7525,I0c2fe374
* changes:
Allow to trace REST requests that match a URI regex by config
Allow to configure automatic tracing by project or calling user
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index ecc2ba0..1601d7a 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4552,6 +4552,52 @@
+
By default, true.
+[[tracing.traceid]]
+==== Subsection tracing.<trace-id>
+
+There can be multiple `tracing.<trace-id>` subsections to configure
+automatic tracing of requests. To be traced a request must match all
+conditions of one `tracing.<trace-id>` subsection. The subsection name
+is used as trace ID. Using this trace ID administrators can find
+matching log entries.
+
+[[tracing.traceid.requestType]]tracing.<trace-id>.requestType::
++
+Type of request for which request tracing should be always enabled (can
+be `GIT_RECEIVE`, `GIT_UPLOAD`, `REST` and `SSH`).
++
+May be specified multiple times.
++
+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
+enabled.
++
+May be specified multiple times.
++
+By default, unset (all accounts are matched).
+
+[[tracing.traceid.projectPattern]]tracing.<trace-id>.projectPattern::
++
+Regular expression to match project names for which request tracing
+should be always enabled.
++
+May be specified multiple times.
++
+By default, unset (all projects are matched).
+
[[trackingid]]
=== Section trackingid
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 1ae39e1..773f712 100644
--- a/java/com/google/gerrit/server/TraceRequestListener.java
+++ b/java/com/google/gerrit/server/TraceRequestListener.java
@@ -14,12 +14,215 @@
package com.google.gerrit.server;
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.logging.RequestId;
+import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.Optional;
+import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+/**
+ * Request listener that sets additional logging tags and enables tracing automatically if the
+ * request matches any tracing configuration in gerrit.config (see description of
+ * 'tracing.<trace-id>' subsection in config-gerrit.txt).
+ */
@Singleton
public class TraceRequestListener implements RequestListener {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final Config cfg;
+ private final ImmutableList<TraceConfig> traceConfigs;
+
+ @Inject
+ TraceRequestListener(@GerritServerConfig Config cfg) {
+ this.cfg = cfg;
+ this.traceConfigs = parseTraceConfigs();
+ }
+
@Override
public void onRequest(RequestInfo requestInfo) {
requestInfo.project().ifPresent(p -> requestInfo.traceContext().addTag("project", p));
+ traceConfigs.stream()
+ .filter(traceConfig -> traceConfig.matches(requestInfo))
+ .forEach(
+ traceConfig ->
+ requestInfo
+ .traceContext()
+ .forceLogging()
+ .addTag(RequestId.Type.TRACE_ID, traceConfig.traceId()));
+ }
+
+ private ImmutableList<TraceConfig> parseTraceConfigs() {
+ ImmutableList.Builder<TraceConfig> traceConfigs = ImmutableList.builder();
+
+ for (String traceId : cfg.getSubsections("tracing")) {
+ try {
+ 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());
+ } catch (ConfigInvalidException e) {
+ logger.atWarning().log("Ignoring invalid tracing configuration:\n %s", e.getMessage());
+ }
+ }
+
+ return traceConfigs.build();
+ }
+
+ private ImmutableSet<String> parseRequestTypes(String traceId) {
+ 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");
+ for (String account : accounts) {
+ Optional<Account.Id> accountId = Account.Id.tryParse(account);
+ if (!accountId.isPresent()) {
+ throw new ConfigInvalidException(
+ String.format(
+ "Invalid tracing config ('tracing.%s.account = %s'): invalid account ID",
+ traceId, account));
+ }
+ accountIds.add(accountId.get());
+ }
+ return accountIds.build();
+ }
+
+ private ImmutableSet<Pattern> parseProjectPatterns(String traceId) throws ConfigInvalidException {
+ 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 {
+ patterns.add(Pattern.compile(patternRegEx));
+ } catch (PatternSyntaxException e) {
+ throw new ConfigInvalidException(
+ String.format(
+ "Invalid tracing config ('tracing.%s.%s = %s'): %s",
+ traceId, name, patternRegEx, e.getMessage()));
+ }
+ }
+ return patterns.build();
+ }
+
+ @AutoValue
+ abstract static class TraceConfig {
+ /** ID for the trace */
+ abstract String traceId();
+
+ /** 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();
+
+ /** pattern matching projects names */
+ abstract ImmutableSet<Pattern> projectPatterns();
+
+ static Builder builder() {
+ return new AutoValue_TraceRequestListener_TraceConfig.Builder();
+ }
+
+ /**
+ * Whether this trace config matches a given request.
+ *
+ * @param requestInfo request info
+ * @return whether this trace config matches
+ */
+ boolean matches(RequestInfo requestInfo) {
+ // If in the trace config request types are set and none of them matches, then the request is
+ // not matched.
+ if (!requestTypes().isEmpty()
+ && requestTypes().stream()
+ .noneMatch(type -> type.equalsIgnoreCase(requestInfo.requestType()))) {
+ 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()) {
+ try {
+ if (accountIds().stream()
+ .noneMatch(id -> id.equals(requestInfo.callingUser().getAccountId()))) {
+ return false;
+ }
+ } catch (UnsupportedOperationException e) {
+ // The calling user is not logged in, hence it cannot match an account.
+ return false;
+ }
+ }
+
+ // If in the trace config project patterns are set and none of them matches, then the request
+ // is not matched.
+ if (!projectPatterns().isEmpty()) {
+ if (!requestInfo.project().isPresent()) {
+ // The request is not for a project, hence it cannot match a project pattern.
+ return false;
+ }
+
+ if (projectPatterns().stream()
+ .noneMatch(p -> p.matcher(requestInfo.project().get().get()).matches())) {
+ return false;
+ }
+ }
+
+ // 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;
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+ abstract Builder traceId(String traceId);
+
+ 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);
+
+ abstract TraceConfig build();
+ }
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
index 50d6dff..3a01517 100644
--- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
@@ -367,6 +367,218 @@
assertThat(testPerformanceLogger.logEntries()).isEmpty();
}
+ @Test
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "new12")
+ public void traceProject() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new12");
+ 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("new12");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "new.*")
+ public void traceProjectMatchRegEx() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new13");
+ 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("new13");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "foo.*")
+ public void traceProjectNoMatch() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new13");
+ 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("new13");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "][")
+ public void traceProjectInvalidRegEx() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new14");
+ 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("new14");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.account", value = "1000000")
+ public void traceAccount() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new15");
+ 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("new15");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.account", value = "1000001")
+ public void traceAccountNoMatch() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new16");
+ 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("new16");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.account", value = "999")
+ public void traceAccountNotFound() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new17");
+ 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("new17");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.account", value = "invalid")
+ public void traceAccountInvalidId() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new18");
+ 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("new18");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestType", value = "REST")
+ public void traceRequestType() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new19");
+ 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("new19");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestType", value = "SSH")
+ public void traceRequestTypeNoMatch() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new20");
+ 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("new20");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestType", value = "FOO")
+ public void traceProjectInvalidRequestType() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new21");
+ 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("new21");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.account", value = "1000000")
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "new.*")
+ public void traceProjectForAccount() throws Exception {
+ RestResponse response = adminRestSession.put("/projects/new22");
+ 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("new22");
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.account", value = "1000000")
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "foo.*")
+ public void traceProjectForAccountNoProjectMatch() 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.account", value = "1000001")
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "new.*")
+ public void traceProjectForAccountNoAccountMatch() 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");
+ }
+
+ @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);
