opendev/gerrit
Code Issues Proposed changes

Instrument ChangeJson to estimate parallelization performance upside

Browse Source 
Change-Id: I9b559b00a1bbf0e7b588ddfff5062268df28ab9a
This commit is contained in:
Patrick Hiesel
2018-04-06 10:20:05 +02:00
parent 83aaf97974
commit 1a4041f0c2
2 changed files with 93 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
Documentation/metrics.txt
View File

@@ -55,6 +55,12 @@ objects needing finalization.
* `http/server/rest_api/server_latency`: REST API call latency by view. * `http/server/rest_api/server_latency`: REST API call latency by view.
* `http/server/rest_api/response_bytes`: Size of REST API response on network * `http/server/rest_api/response_bytes`: Size of REST API response on network
(may be gzip compressed) by view. (may be gzip compressed) by view.
* `http/server/rest_api/change_json/to_change_info_latency`: Latency for
toChangeInfo invocations in ChangeJson.
* `http/server/rest_api/change_json/to_change_infos_latency`: Latency for
toChangeInfos invocations in ChangeJson.
* `http/server/rest_api/change_json/format_query_results_latency`: Latency for
formatQueryResults invocations in ChangeJson.
=== Query === Query

126
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -87,6 +87,10 @@ import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.index.query.QueryResult; import com.google.gerrit.index.query.QueryResult;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -202,6 +206,35 @@ public class ChangeJson {
ChangeJson create(Iterable<ListChangesOption> options); ChangeJson create(Iterable<ListChangesOption> options);
} }
@Singleton
private static class Metrics {
private final Timer0 toChangeInfoLatency;
private final Timer0 toChangeInfosLatency;
private final Timer0 formatQueryResultsLatency;
@Inject
Metrics(MetricMaker metricMaker) {
toChangeInfoLatency =
metricMaker.newTimer(
"http/server/rest_api/change_json/to_change_info_latency",
new Description("Latency for toChangeInfo invocations in ChangeJson")
.setCumulative()
.setUnit(Units.MILLISECONDS));
toChangeInfosLatency =
metricMaker.newTimer(
"http/server/rest_api/change_json/to_change_infos_latency",
new Description("Latency for toChangeInfos invocations in ChangeJson")
.setCumulative()
.setUnit(Units.MILLISECONDS));
formatQueryResultsLatency =
metricMaker.newTimer(
"http/server/rest_api/change_json/format_query_results_latency",
new Description("Latency for formatQueryResults invocations in ChangeJson")
.setCumulative()
.setUnit(Units.MILLISECONDS));
}
}
private final Provider<ReviewDb> db; private final Provider<ReviewDb> db;
private final Provider<CurrentUser> userProvider; private final Provider<CurrentUser> userProvider;
private final AnonymousUser anonymous; private final AnonymousUser anonymous;
@@ -228,6 +261,7 @@ public class ChangeJson {
private final ApprovalsUtil approvalsUtil; private final ApprovalsUtil approvalsUtil;
private final RemoveReviewerControl removeReviewerControl; private final RemoveReviewerControl removeReviewerControl;
private final TrackingFooters trackingFooters; private final TrackingFooters trackingFooters;
private final Metrics metrics;
private boolean lazyLoad = true; private boolean lazyLoad = true;
private AccountLoader accountLoader; private AccountLoader accountLoader;
private FixInput fix; private FixInput fix;
@@ -260,6 +294,7 @@ public class ChangeJson {
ApprovalsUtil approvalsUtil, ApprovalsUtil approvalsUtil,
RemoveReviewerControl removeReviewerControl, RemoveReviewerControl removeReviewerControl,
TrackingFooters trackingFooters, TrackingFooters trackingFooters,
Metrics metrics,
@Assisted Iterable<ListChangesOption> options) { @Assisted Iterable<ListChangesOption> options) {
this.db = db; this.db = db;
this.userProvider = user; this.userProvider = user;
@@ -285,8 +320,9 @@ public class ChangeJson {
this.indexes = indexes; this.indexes = indexes;
this.approvalsUtil = approvalsUtil; this.approvalsUtil = approvalsUtil;
this.removeReviewerControl = removeReviewerControl; this.removeReviewerControl = removeReviewerControl;
this.options = Sets.immutableEnumSet(options);
this.trackingFooters = trackingFooters; this.trackingFooters = trackingFooters;
this.metrics = metrics;
this.options = Sets.immutableEnumSet(options);
} }
public ChangeJson lazyLoad(boolean load) { public ChangeJson lazyLoad(boolean load) {
@@ -360,20 +396,22 @@ public class ChangeJson {
public List<List<ChangeInfo>> formatQueryResults(List<QueryResult<ChangeData>> in) public List<List<ChangeInfo>> formatQueryResults(List<QueryResult<ChangeData>> in)
throws OrmException { throws OrmException {
accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); try (Timer0.Context ignored = metrics.formatQueryResultsLatency.start()) {
ensureLoaded(FluentIterable.from(in).transformAndConcat(QueryResult::entities)); accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
ensureLoaded(FluentIterable.from(in).transformAndConcat(QueryResult::entities));
List<List<ChangeInfo>> res = Lists.newArrayListWithCapacity(in.size()); List<List<ChangeInfo>> res = Lists.newArrayListWithCapacity(in.size());
Map<Change.Id, ChangeInfo> out = new HashMap<>(); Map<Change.Id, ChangeInfo> out = new HashMap<>();
for (QueryResult<ChangeData> r : in) { for (QueryResult<ChangeData> r : in) {
List<ChangeInfo> infos = toChangeInfo(out, r.entities()); List<ChangeInfo> infos = toChangeInfos(out, r.entities());
if (!infos.isEmpty() && r.more()) { if (!infos.isEmpty() && r.more()) {
infos.get(infos.size() - 1)._moreChanges = true; infos.get(infos.size() - 1)._moreChanges = true;
}
res.add(infos);
} }
res.add(infos); accountLoader.fill();
return res;
} }
accountLoader.fill();
return res;
} }
public List<ChangeInfo> formatChangeDatas(Collection<ChangeData> in) throws OrmException { public List<ChangeInfo> formatChangeDatas(Collection<ChangeData> in) throws OrmException {
@@ -410,37 +448,39 @@ public class ChangeJson {
return options.contains(option); return options.contains(option);
} }
private List<ChangeInfo> toChangeInfo(Map<Change.Id, ChangeInfo> out, List<ChangeData> changes) { private List<ChangeInfo> toChangeInfos(Map<Change.Id, ChangeInfo> out, List<ChangeData> changes) {
List<ChangeInfo> info = Lists.newArrayListWithCapacity(changes.size()); try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) {
for (ChangeData cd : changes) { List<ChangeInfo> info = Lists.newArrayListWithCapacity(changes.size());
ChangeInfo i = out.get(cd.getId()); for (ChangeData cd : changes) {
if (i == null) { ChangeInfo i = out.get(cd.getId());
try { if (i == null) {
i = toChangeInfo(cd, Optional.empty()); try {
} catch (PatchListNotAvailableException i = toChangeInfo(cd, Optional.empty());
| GpgException } catch (PatchListNotAvailableException
| OrmException | GpgException
| IOException | OrmException
| PermissionBackendException | IOException
| RuntimeException e) { | PermissionBackendException
if (has(CHECK)) { | RuntimeException e) {
i = checkOnly(cd); if (has(CHECK)) {
} else if (e instanceof NoSuchChangeException) { i = checkOnly(cd);
log.info( } else if (e instanceof NoSuchChangeException) {
"NoSuchChangeException: Omitting corrupt change " log.info(
+ cd.getId() "NoSuchChangeException: Omitting corrupt change "
+ " from results. Seems to be stale in the index."); + cd.getId()
continue; + " from results. Seems to be stale in the index.");
} else { continue;
log.warn("Omitting corrupt change " + cd.getId() + " from results", e); } else {
continue; log.warn("Omitting corrupt change " + cd.getId() + " from results", e);
continue;
}
} }
out.put(cd.getId(), i);
} }
out.put(cd.getId(), i); info.add(i);
} }
info.add(i); return info;
} }
return info;
} }
private ChangeInfo checkOnly(ChangeData cd) { private ChangeInfo checkOnly(ChangeData cd) {
@@ -489,6 +529,14 @@ public class ChangeJson {
private ChangeInfo toChangeInfo(ChangeData cd, Optional<PatchSet.Id> limitToPsId) private ChangeInfo toChangeInfo(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException, throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException,
IOException { IOException {
try (Timer0.Context ignored = metrics.toChangeInfoLatency.start()) {
return toChangeInfoImpl(cd, limitToPsId);
}
}
private ChangeInfo toChangeInfoImpl(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException,
IOException {
ChangeInfo out = new ChangeInfo(); ChangeInfo out = new ChangeInfo();
CurrentUser user = userProvider.get(); CurrentUser user = userProvider.get();