Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Oct 23, 2020

Related to #516

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

Using meters should improve this since:

  1. We can more easily visualize the current revision state for each watcher (logs only provide whether the update occurred during a certain time frame)
  2. 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.

@jrhee17 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
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@Override
public MeterRegistry meterRegistry() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add final?


/**
* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@minwoox
Copy link
Member

minwoox commented Mar 4, 2021

This looks good!

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

codecov bot commented Apr 15, 2021

Codecov Report

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

Impacted file tree graph

@@             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.

@jrhee17 jrhee17 force-pushed the feature/introduce-client-meter branch 2 times, most recently from f04c394 to e32a68e Compare April 16, 2021 11:43
@jrhee17 jrhee17 marked this pull request as ready for review April 16, 2021 12:00
@jrhee17 jrhee17 requested a review from ikhoon April 16, 2021 12:00
@minwoox
Copy link
Member

minwoox commented Apr 19, 2021

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 jrhee17 force-pushed the feature/introduce-client-meter branch from 268209c to fa61474 Compare April 26, 2021 13:46
@jrhee17
Copy link
Contributor Author

jrhee17 commented Apr 28, 2021

*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:

  1. Provide the same meterRegistry should be provided for both ClientFactory, CentralDogma
  2. 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

  1. 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
  2. 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 jrhee17 force-pushed the feature/introduce-client-meter branch from fa61474 to 075c264 Compare June 3, 2021 15:20
@jrhee17
Copy link
Contributor Author

jrhee17 commented Jun 3, 2021

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

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ikhoon ikhoon added this to the 0.51.0 milestone Jun 4, 2021
@trustin
Copy link
Member

trustin commented Jun 5, 2021

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
Copy link
Contributor Author

jrhee17 commented Jun 19, 2021

Ready for the next round of reviews 🙏

@jrhee17 jrhee17 requested a review from ikhoon June 19, 2021 08:56
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! left some questions. 😄

}

CompletableFutures.allAsList(futures).thenAccept(results -> {
final boolean result = results.stream().allMatch(x -> x);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@minwoox minwoox modified the milestones: 0.51.0, 0.52.0 Jun 28, 2021
@jrhee17 jrhee17 requested a review from minwoox July 3, 2021 05:31
@minwoox minwoox modified the milestones: 0.52.0, 0.52.1 Aug 24, 2021
@trustin
Copy link
Member

trustin commented Mar 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants