Introduce client-side micrometer, start with exposing watcher revision metrics #542

jrhee17 · 2020-10-23T08:38:31Z

Related to #516

Motivation
We often add logs to check if each watcher has called its notifiers successfully.

Using meters should improve this since:

We can more easily visualize the current revision state for each watcher (logs only provide whether the update occurred during a certain time frame)
Integration with our stack (prometheus/grafana) provides a better interface to check if any servers/watchers have failed to update.

Rather than migrating our 50+ watchers, I would like to propose centraldogma provides this functionality out of the box.

Modifications

Allow *CentralDogmaBuilder to specify MeterRegistry
Add MeterRegistry to constructors for *CentralDogma
Add a Gauge for watcher revisions

Known Limitations

Watchers with duplicate tags won't report metrics (metrics for a watcher with the same project/repository/path will be ignored)
- I suppose later if needed we can introduce an identifier for each Watcher if someone needs this functionality. Hard to think of a scenario where multiple watchers of this nature will be needed though.
*CentralDogma.meterRegistry and ClientFactory.meterRegistry might be different, which might cause confusion.
- I guess later if needed *CentralDogma can use ClientFactory.meterRegistry by default if not specified. I think this can also be supported without any breaking changes in the future though.

ikhoon

Oops... I missed this PR. Mostly LGTM. 🙇‍♂️

ikhoon · 2020-11-23T12:29:01Z

client/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogma.java

@@ -201,4 +205,9 @@ protected final ScheduledExecutorService executor() {
            return CompletableFuture.completedFuture(revision);
        }
    }
+
+    @Override
+    public MeterRegistry meterRegistry() {


Add final?

ikhoon · 2020-11-23T12:29:55Z

client/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogma.java


    /**
     * Creates a new instance.
     *
     * @param executor the {@link ScheduledExecutorService} which will be used for scheduling the tasks
     *                 related with automatic retries.
     */
-    protected AbstractCentralDogma(ScheduledExecutorService executor) {
+    protected AbstractCentralDogma(ScheduledExecutorService executor, MeterRegistry meterRegistry) {


This could cause breaking changes.
Could you add a new one?

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java

minwoox · 2021-03-04T03:05:50Z

This looks good!

codecov · 2021-04-15T12:18:10Z

Codecov Report

Merging #542 (b92985d) into master (3799e35) will increase coverage by 0.05%.
The diff coverage is 79.48%.

@@             Coverage Diff              @@
##             master     #542      +/-   ##
============================================
+ Coverage     70.22%   70.28%   +0.05%     
- Complexity     3354     3361       +7     
============================================
  Files           336      336              
  Lines         13359    13388      +29     
  Branches       1443     1447       +4     
============================================
+ Hits           9382     9410      +28     
- Misses         3084     3085       +1     
  Partials        893      893

Impacted Files	Coverage Δ
...com/linecorp/centraldogma/client/CentralDogma.java	`57.14% <0.00%> (-2.12%)`	⬇️
...ient/armeria/legacy/LegacyCentralDogmaBuilder.java	`82.14% <50.00%> (-3.05%)`	⬇️
...gma/client/armeria/ArmeriaCentralDogmaBuilder.java	`76.47% <50.00%> (-4.78%)`	⬇️
...ntraldogma/client/AbstractCentralDogmaBuilder.java	`73.33% <50.00%> (-0.81%)`	⬇️
...corp/centraldogma/client/AbstractCentralDogma.java	`68.57% <60.00%> (-2.40%)`	⬇️
.../centraldogma/internal/client/AbstractWatcher.java	`80.98% <95.45%> (+1.67%)`	⬆️
...ogma/client/armeria/legacy/LegacyCentralDogma.java	`82.74% <100.00%> (ø)`
...ntraldogma/client/armeria/ArmeriaCentralDogma.java	`72.35% <100.00%> (ø)`
...nal/client/ReplicationLagTolerantCentralDogma.java	`87.55% <100.00%> (ø)`
...erver/internal/storage/PurgeSchedulingService.java	`70.11% <0.00%> (-6.90%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3799e35...b92985d. Read the comment docs.

minwoox · 2021-04-19T07:42:45Z

I guess later if needed *CentralDogma can use ClientFactory.meterRegistry by default if not specified.

How about implementing it in this PR?

*CentralDogma.meterRegistry and ClientFactory.meterRegistry might be different,

Shouldn't we warn in this case?

jrhee17 · 2021-04-28T23:46:35Z

*CentralDogma.meterRegistry and ClientFactory.meterRegistry might be different,
Shouldn't we warn in this case?

I tried this out, but this means for the user either:

Provide the same meterRegistry should be provided for both ClientFactory, CentralDogma
Or set meterRegistry for ClientFactory only relying on the fallback logic

I'm thinking whether simply removing the option to set meterRegistry in CentralDogma and relying on ClientFactory only may provide a more intuitive API

Let me know what you think

Update 2021/6/4

I realized AbstractCentralDogma is public, and we can't avoid users setting meterRegistry directory on *CentralDogma without introducing a breaking change.

I guess this boils down to

Do we introduce a slight breaking change at the cost of disallowing users from setting their own meterRegistry to *CentralDogma and rely on ClientFactory.meterRegistry
Do we just let users set meterRegistry which may be different from ClientFactory.meterRegistry, and just let users know via warning logs if the two are different

jrhee17 · 2021-06-03T15:24:12Z

rebased once - gentle ping, as I still think this PR has value

ikhoon

Sorry for the late review. Mostly looking good to me!

client/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogmaBuilder.java

client/java/src/main/java/com/linecorp/centraldogma/internal/client/AbstractWatcher.java

trustin · 2021-06-05T05:46:42Z

I'm thinking whether simply removing the option to set meterRegistry in CentralDogma and relying on ClientFactory only may provide a more intuitive API

Let me know what you think

+1 for always using ClientFactory.meterRegistry(). How about adding disableMetrics() instead for those who want to opt out of metrics? (although not very likely)

jrhee17 · 2021-06-19T08:55:57Z

Ready for the next round of reviews 🙏

minwoox

Thanks! left some questions. 😄

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java

client/java/src/main/java/com/linecorp/centraldogma/internal/client/AbstractWatcher.java

minwoox · 2021-06-24T05:25:27Z

client/java/src/main/java/com/linecorp/centraldogma/internal/client/AbstractWatcher.java

        }
+
+        CompletableFutures.allAsList(futures).thenAccept(results -> {
+            final boolean result = results.stream().allMatch(x -> x);


Shouldn't we update the latestNotifiedRevision regardless of a listener throws an exception?

I've added another metric which updates the revision irregardless of whether the notify succeeds.

I think the two metrics combined can give users a good idea of whether properties have been propagated correctly.

…cess

trustin · 2023-03-08T10:18:36Z

What's the status of this PR since then? 😅

jrhee17 changed the title ~~introduce client-side micrometer, start with exposing watcher revision metrics~~ Introduce client-side micrometer, start with exposing watcher revision metrics Oct 23, 2020

ikhoon reviewed Nov 23, 2020

View reviewed changes

ikhoon added the new feature label Nov 23, 2020

jrhee17 force-pushed the feature/introduce-client-meter branch from 1c3ad2b to a9cfd8e Compare April 15, 2021 11:05

jrhee17 force-pushed the feature/introduce-client-meter branch 2 times, most recently from f04c394 to e32a68e Compare April 16, 2021 11:43

jrhee17 marked this pull request as ready for review April 16, 2021 12:00

jrhee17 requested review from minwoox and trustin as code owners April 16, 2021 12:00

jrhee17 requested a review from ikhoon April 16, 2021 12:00

jrhee17 force-pushed the feature/introduce-client-meter branch from 268209c to fa61474 Compare April 26, 2021 13:46

jrhee17 added 7 commits June 3, 2021 23:56

minimal impl

043a5b6

add javadocs, overload ctor for AbstractCentralDogma

e005295

address checkstyle

10be21d

add it test, update revision when notify succeeds

125e38a

use clientFactory meterRegistry by default

f8cf421

warn if the meterRegistry is different

d2e79c5

fix checkstyle from rebasing

075c264

jrhee17 force-pushed the feature/introduce-client-meter branch from fa61474 to 075c264 Compare June 3, 2021 15:20

ikhoon reviewed Jun 4, 2021

View reviewed changes

client/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogmaBuilder.java Outdated Show resolved Hide resolved

client/java/src/main/java/com/linecorp/centraldogma/internal/client/AbstractWatcher.java Outdated Show resolved Hide resolved

ikhoon added this to the 0.51.0 milestone Jun 4, 2021

jrhee17 added 3 commits June 19, 2021 15:02

Merge branch 'master' into feature/introduce-client-meter

c0ed9a8

don't let users specify meterRegistry from builders

c433afd

fix broken test, remove unused imports

23ad16b

jrhee17 added 3 commits June 19, 2021 17:30

first try at introducing disableMetrics

aa1f7b2

allow meterRegistry to be nullable for a cleaner CentralDogma interface

9300bd0

remove unused imports

711b232

jrhee17 requested a review from ikhoon June 19, 2021 08:56

fix failing test

26cb807

minwoox reviewed Jun 24, 2021

View reviewed changes

minwoox modified the milestones: 0.51.0, 0.52.0 Jun 28, 2021

jrhee17 added 3 commits July 3, 2021 14:17

prefer counter over gauge, fix javadoc

7f4e5d2

add watcher rev which updates the revision irregardless of notify suc…

6b44898

…cess

address checkstyle long line

b92985d

jrhee17 requested a review from minwoox July 3, 2021 05:31

minwoox modified the milestones: 0.52.0, 0.52.1 Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce client-side micrometer, start with exposing watcher revision metrics #542

Introduce client-side micrometer, start with exposing watcher revision metrics #542

jrhee17 commented Oct 23, 2020 •

edited

ikhoon left a comment

ikhoon Nov 23, 2020

ikhoon Nov 23, 2020

minwoox commented Mar 4, 2021

codecov bot commented Apr 15, 2021 •

edited

minwoox commented Apr 19, 2021

jrhee17 commented Apr 28, 2021 •

edited

jrhee17 commented Jun 3, 2021

ikhoon left a comment

trustin commented Jun 5, 2021

jrhee17 commented Jun 19, 2021

minwoox left a comment

minwoox Jun 24, 2021

jrhee17 Jul 3, 2021

trustin commented Mar 8, 2023

Introduce client-side micrometer, start with exposing watcher revision metrics #542

Are you sure you want to change the base?

Introduce client-side micrometer, start with exposing watcher revision metrics #542

Conversation

jrhee17 commented Oct 23, 2020 • edited

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Nov 23, 2020

Choose a reason for hiding this comment

ikhoon Nov 23, 2020

Choose a reason for hiding this comment

minwoox commented Mar 4, 2021

codecov bot commented Apr 15, 2021 • edited

Codecov Report

minwoox commented Apr 19, 2021

jrhee17 commented Apr 28, 2021 • edited

jrhee17 commented Jun 3, 2021

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented Jun 5, 2021

jrhee17 commented Jun 19, 2021

minwoox left a comment

Choose a reason for hiding this comment

minwoox Jun 24, 2021

Choose a reason for hiding this comment

jrhee17 Jul 3, 2021

Choose a reason for hiding this comment

trustin commented Mar 8, 2023

jrhee17 commented Oct 23, 2020 •

edited

codecov bot commented Apr 15, 2021 •

edited

jrhee17 commented Apr 28, 2021 •

edited