Record metrics when INIT revision or an old revision is used #874

minwoox · 2023-08-28T08:53:50Z

Motivation:
Before we apply #681 that removes old commits, it would be nice if we could find any usages that access data with the INIT Revision or revisions that are more than 5000 (default minimum number of commit retentions) commits ahead from the head.

Modifications:

Add a counter that is increased when INIT Revision or revisions that are more than 5000 ahead from the head are used.

Result:

Track the number of usages.
This commit will be reverted after we find the usage and fix it.

Motivation: Before we apply line#681 that removes old commits, it would be nice if we can find any usages that access data with the INIT Revision or revisions that are more than 5000 (default minimum number of commit retentions) commits ahead from the head. Modifications: - Add a counter that is incresed when INIT Revision or revisions that are more than 5000 ahead from the head are used. Result: - Track the number of usages. - This commit will be reverted after we find the usage and fix it.

jrhee17 · 2023-08-28T09:50:04Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java

+            ServiceRequestContext ctx, String projectName, String repoName,
+            Revision normalized, Revision head) {
+        if (normalized.major() == 1 || head.major() - normalized.major() >= 5000) {
+            ctx.log().whenComplete().thenAccept(log -> {


~~Question) Do we want to differentiate between cases using 1 vs. >5000?~~
~~Imagining that the counter goes up, I wonder if we want to know which case needs handling.~~

Nevermind, I realized you added the revision as a tag

jrhee17

Left some minor comments, but basically looks good 👍

jrhee17 · 2023-08-28T10:01:14Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java

+                                Tag.of("repo", repoName),
+                                Tag.of("service", firstNonNull(log.serviceName(), "none")),
+                                Tag.of("method", log.name()),
+                                Tag.of("normalized", Integer.toString(normalized.major())),


Instead of adding a tag which can increase cardinality, what do you think of just adding a distribution?

i.e. If there is a repo with head version 1M, this is the possible cardinality worse case.

ctx.meterRegistry().summary("").record(head.major() - normalized.major()); ctx.meterRegistry().counter("", "init.major", String.valueOf(normalized.major() == 1)).increment();

I have added the metric with the assumption that this will be short-lived and there will not be many requests that are caught by the condition.
However, there's no reason not to decrease the cardinality so let me use the distribution. Thanks!

I have added the metric with the assumption that this will be short-lived and there will not be many requests that are caught by the condition.

I agree, but I also thought this code will also need to go to production env. to confirm our hypothesis 😄

That is correct. 🤣

.../src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java

codecov · 2023-08-28T11:00:10Z

Codecov Report

Patch coverage: 83.90% and project coverage change: +0.04% 🎉

Comparison is base (6b10542) 65.69% compared to head (dce6327) 65.73%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #874      +/-   ##
============================================
+ Coverage     65.69%   65.73%   +0.04%     
- Complexity     3350     3351       +1     
============================================
  Files           358      358              
  Lines         13893    13954      +61     
  Branches       1496     1502       +6     
============================================
+ Hits           9127     9173      +46     
- Misses         3914     3924      +10     
- Partials        852      857       +5

Files Changed	Coverage Δ
...dogma/server/internal/api/RepositoryServiceV1.java	`73.43% <70.37%> (-2.24%)`	⬇️
...erver/internal/mirror/DefaultMirroringService.java	`67.67% <71.42%> (-0.04%)`	⬇️
...raldogma/server/internal/api/ContentServiceV1.java	`86.98% <100.00%> (+0.41%)`	⬆️
...raldogma/server/internal/mirror/MirroringTask.java	`100.00% <100.00%> (ø)`
...erver/internal/thrift/CentralDogmaServiceImpl.java	`76.59% <100.00%> (+1.94%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jrhee17

Looks good to me 👍 Thanks @minwoox ! 🙇 👍 🚀

minwoox · 2023-08-29T02:48:48Z

common/build.gradle

@@ -22,5 +22,7 @@ if (tasks.findByName('trimShadedJar')) {
        keep "class com.linecorp.centraldogma.internal.shaded.caffeine.** { *; }"
        // Prevent ProGuard from removing all enum values from Option because otherwise it becomes a non-enum class.
        keep "class com.linecorp.centraldogma.internal.shaded.jsonpath.Option { *; }"
+
+        dontnote


This reduces the verbosity of ProGuardTask when running in parallel.
The original comment: https://github.com/line/armeria/pull/2172/files#r332367951

Please add a line comment about it.

jrhee17

New changes still look good 👍

trustin · 2023-08-29T02:49:12Z

common/build.gradle

@@ -22,5 +22,7 @@ if (tasks.findByName('trimShadedJar')) {
        keep "class com.linecorp.centraldogma.internal.shaded.caffeine.** { *; }"
        // Prevent ProGuard from removing all enum values from Option because otherwise it becomes a non-enum class.
        keep "class com.linecorp.centraldogma.internal.shaded.jsonpath.Option { *; }"
+
+        dontnote


Please add a line comment about it.

trustin · 2023-08-29T02:51:32Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java

+        if (normalized.major() == 1) {
+            ctx.log().whenComplete().thenAccept(
+                    log -> ctx.meterRegistry()
+                              .counter("init.revisions", generateTags(projectName, repoName, log).build())


revisions.init?

trustin · 2023-08-29T02:51:41Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java

+        if (head.major() - normalized.major() >= 5000) {
+            ctx.log().whenComplete().thenAccept(
+                    log -> ctx.meterRegistry()
+                              .summary("old.revisions",


revisions.old?

trustin · 2023-08-29T02:56:24Z

server/src/test/java/com/linecorp/centraldogma/server/MetricsTest.java

+        // Trigger old revision recording
+        jsonNodeWatchRequest.start(Revision.INIT).join();
+        content = dogma.httpClient().get("/monitor/metrics").aggregate().join().contentUtf8();
+        assertThat(content).contains("init_revisions");


Can we also add a similar test for old_revisions? Should check if the meters with init=true and init=false both exists.

All addressed. PTAL. 😉

ikhoon

Thanks, @minwoox

ikhoon · 2023-08-30T05:53:56Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java

+    }
+
+    public static void increaseCounterIfOldRevisionUsed(
+            ServiceRequestContext ctx, String projectName, String repoName,


nit: It may be simpler.

Suggested change

ServiceRequestContext ctx, String projectName, String repoName,

ServiceRequestContext ctx, Repository repository,

ikhoon · 2023-08-30T05:59:23Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java

+            ServiceRequestContext ctx, String projectName, String repoName,
+            Revision normalized, Revision head) {
+        if (normalized.major() == 1) {
+            ctx.log().whenComplete().thenAccept(


nit: ctx.log.whenRequestComplete() could be used instead since RequestLogProperty.NAME is a request property. But I'm okay with the current implementation for simplicity.

Thanks! All addressed. 😉

minwoox · 2023-09-04T02:17:32Z

Thanks for reviewing. 😉

minwoox added the miscellaneous label Aug 28, 2023

minwoox requested review from ikhoon, jrhee17 and trustin as code owners August 28, 2023 08:53

jrhee17 reviewed Aug 28, 2023

View reviewed changes

Address comments from @jrhee17

a302f38

Update tag

37e6a85

jrhee17 approved these changes Aug 28, 2023

View reviewed changes

minwoox added 5 commits August 29, 2023 11:35

Add projectName to mirroring metrics

82fa47c

lint

2b5fd93

Fix tests

a89d781

Fix again

24ef8be

Add dontnote

f295224

minwoox commented Aug 29, 2023

View reviewed changes

jrhee17 approved these changes Aug 29, 2023

View reviewed changes

trustin reviewed Aug 29, 2023

View reviewed changes

Address comments from @trustin

d1d36a2

ikhoon approved these changes Aug 30, 2023

View reviewed changes

Address comments from @ikhoon

dce6327

minwoox merged commit c0fdd1a into line:main Sep 4, 2023
10 of 11 checks passed

minwoox deleted the old_revision branch September 4, 2023 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record metrics when INIT revision or an old revision is used #874

Record metrics when INIT revision or an old revision is used #874

minwoox commented Aug 28, 2023

jrhee17 Aug 28, 2023 •

edited

Loading

jrhee17 left a comment

jrhee17 Aug 28, 2023

minwoox Aug 28, 2023

jrhee17 Aug 28, 2023

minwoox Aug 28, 2023

codecov bot commented Aug 28, 2023 •

edited

Loading

jrhee17 left a comment

minwoox Aug 29, 2023

trustin Aug 29, 2023

jrhee17 left a comment

trustin Aug 29, 2023

trustin Aug 29, 2023

trustin Aug 29, 2023

trustin Aug 29, 2023

minwoox Aug 29, 2023

ikhoon left a comment

ikhoon Aug 30, 2023

ikhoon Aug 30, 2023

minwoox Sep 1, 2023

minwoox commented Sep 4, 2023

	ServiceRequestContext ctx, String projectName, String repoName,
	ServiceRequestContext ctx, Repository repository,

Record metrics when INIT revision or an old revision is used #874

Record metrics when INIT revision or an old revision is used #874

Conversation

minwoox commented Aug 28, 2023

jrhee17 Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 28, 2023 • edited Loading

Codecov Report

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Sep 4, 2023

jrhee17 Aug 28, 2023 •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading