From 1a4041f0c261219a51c9ab8a92ded873f04e2130 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 6 Apr 2018 10:20:05 +0200 Subject: [PATCH] Instrument ChangeJson to estimate parallelization performance upside Change-Id: I9b559b00a1bbf0e7b588ddfff5062268df28ab9a --- Documentation/metrics.txt | 6 + .../gerrit/server/change/ChangeJson.java | 126 ++++++++++++------ 2 files changed, 93 insertions(+), 39 deletions(-) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index 229c463f11..fa65a0b12f 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -55,6 +55,12 @@ objects needing finalization. * `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 (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 diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 19c9666279..6b5ff993ce 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -87,6 +87,10 @@ import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.Url; 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.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; @@ -202,6 +206,35 @@ public class ChangeJson { ChangeJson create(Iterable 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 db; private final Provider userProvider; private final AnonymousUser anonymous; @@ -228,6 +261,7 @@ public class ChangeJson { private final ApprovalsUtil approvalsUtil; private final RemoveReviewerControl removeReviewerControl; private final TrackingFooters trackingFooters; + private final Metrics metrics; private boolean lazyLoad = true; private AccountLoader accountLoader; private FixInput fix; @@ -260,6 +294,7 @@ public class ChangeJson { ApprovalsUtil approvalsUtil, RemoveReviewerControl removeReviewerControl, TrackingFooters trackingFooters, + Metrics metrics, @Assisted Iterable options) { this.db = db; this.userProvider = user; @@ -285,8 +320,9 @@ public class ChangeJson { this.indexes = indexes; this.approvalsUtil = approvalsUtil; this.removeReviewerControl = removeReviewerControl; - this.options = Sets.immutableEnumSet(options); this.trackingFooters = trackingFooters; + this.metrics = metrics; + this.options = Sets.immutableEnumSet(options); } public ChangeJson lazyLoad(boolean load) { @@ -360,20 +396,22 @@ public class ChangeJson { public List> formatQueryResults(List> in) throws OrmException { - accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); - ensureLoaded(FluentIterable.from(in).transformAndConcat(QueryResult::entities)); + try (Timer0.Context ignored = metrics.formatQueryResultsLatency.start()) { + accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); + ensureLoaded(FluentIterable.from(in).transformAndConcat(QueryResult::entities)); - List> res = Lists.newArrayListWithCapacity(in.size()); - Map out = new HashMap<>(); - for (QueryResult r : in) { - List infos = toChangeInfo(out, r.entities()); - if (!infos.isEmpty() && r.more()) { - infos.get(infos.size() - 1)._moreChanges = true; + List> res = Lists.newArrayListWithCapacity(in.size()); + Map out = new HashMap<>(); + for (QueryResult r : in) { + List infos = toChangeInfos(out, r.entities()); + if (!infos.isEmpty() && r.more()) { + infos.get(infos.size() - 1)._moreChanges = true; + } + res.add(infos); } - res.add(infos); + accountLoader.fill(); + return res; } - accountLoader.fill(); - return res; } public List formatChangeDatas(Collection in) throws OrmException { @@ -410,37 +448,39 @@ public class ChangeJson { return options.contains(option); } - private List toChangeInfo(Map out, List changes) { - List info = Lists.newArrayListWithCapacity(changes.size()); - for (ChangeData cd : changes) { - ChangeInfo i = out.get(cd.getId()); - if (i == null) { - try { - i = toChangeInfo(cd, Optional.empty()); - } catch (PatchListNotAvailableException - | GpgException - | OrmException - | IOException - | PermissionBackendException - | RuntimeException e) { - if (has(CHECK)) { - i = checkOnly(cd); - } else if (e instanceof NoSuchChangeException) { - log.info( - "NoSuchChangeException: Omitting corrupt change " - + cd.getId() - + " from results. Seems to be stale in the index."); - continue; - } else { - log.warn("Omitting corrupt change " + cd.getId() + " from results", e); - continue; + private List toChangeInfos(Map out, List changes) { + try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) { + List info = Lists.newArrayListWithCapacity(changes.size()); + for (ChangeData cd : changes) { + ChangeInfo i = out.get(cd.getId()); + if (i == null) { + try { + i = toChangeInfo(cd, Optional.empty()); + } catch (PatchListNotAvailableException + | GpgException + | OrmException + | IOException + | PermissionBackendException + | RuntimeException e) { + if (has(CHECK)) { + i = checkOnly(cd); + } else if (e instanceof NoSuchChangeException) { + log.info( + "NoSuchChangeException: Omitting corrupt change " + + cd.getId() + + " from results. Seems to be stale in the index."); + continue; + } else { + 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) { @@ -489,6 +529,14 @@ public class ChangeJson { private ChangeInfo toChangeInfo(ChangeData cd, Optional limitToPsId) throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException, IOException { + try (Timer0.Context ignored = metrics.toChangeInfoLatency.start()) { + return toChangeInfoImpl(cd, limitToPsId); + } + } + + private ChangeInfo toChangeInfoImpl(ChangeData cd, Optional limitToPsId) + throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException, + IOException { ChangeInfo out = new ChangeInfo(); CurrentUser user = userProvider.get();