Report watcher metrics related to the latest revision #1094

be-hase · 2025-02-06T06:00:22Z

Additional note

I can't think of a use case for customizing the meter name prefix, so I won't support it at this time.
(In the case of Armeria, there are occasional use cases when using both gRPC and Thrift simultaneously, though...)

Tasks

Get feedback
Write test

CLAassistant · 2025-02-06T06:00:30Z

All committers have signed the CLA.

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

codecov · 2025-02-06T09:19:09Z

Codecov Report

Attention: Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.12%. Comparing base (767eff3) to head (61684c1).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ogma/client/armeria/legacy/LegacyCentralDogma.java	33.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1094      +/-   ##
============================================
+ Coverage     70.07%   70.12%   +0.05%     
- Complexity     4486     4496      +10     
============================================
  Files           453      453              
  Lines         18161    18212      +51     
  Branches       2008     2015       +7     
============================================
+ Hits          12727    12772      +45     
- Misses         4345     4347       +2     
- Partials       1089     1093       +4

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java

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

ikhoon · 2025-02-07T02:06:07Z

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

+    private static final String LATEST_REVISION_METER_NAME = "centraldogma.watcher.latest.revision";
+    private static final String LATEST_COMMIT_TIME_METER_NAME = "centraldogma.watcher.latest.commit.time";


Suggested change

private static final String LATEST_REVISION_METER_NAME = "centraldogma.watcher.latest.revision";

private static final String LATEST_COMMIT_TIME_METER_NAME = "centraldogma.watcher.latest.commit.time";

private static final String LATEST_REVISION_METER_NAME = "centraldogma.client.watcher.latest.revision";

private static final String LATEST_COMMIT_TIME_METER_NAME = "centraldogma.client.watcher.latest.commit.time";

Some people may want to use a different prefix and add custom tags for their metrics.
What do you think of adding a customization point for that?

I realized that the centraldogma-client module has no dependency on Armeria. Actually, we have decided to add Armeria dependency to centraldogma-client but didn't put it in action.

To conclude, let's add Armeria dependency and use MeterIdPrefix, which I proposed in the issue.

I also wrote this in the README, but since this is a relatively unique metric, I couldn’t think of many use cases for customization at this point.

I considered using this, but copying and pasting it didn’t seem ideal, so I decided not to for now.
(That said, it's also not a good idea for centraldogma-client to depend on Armeria.)
MeterIdPrefix in Armeria

And if we want to rename or retag metrics, we can transform them using a meter filter.
Micrometer Meter Filters Documentation

...How about?

I see. How about just adding an infix .client? I want to distinguish it from the server metric. We may add additional metrics under centraldogma.client namespace.

Ah, I overlooked the client infix remark. You're absolutely right. I'll fix it!

be-hase · 2025-02-07T06:46:00Z

( Hmm... For some reason, the CI is failing on Windows... Why...?

jrhee17 · 2025-02-10T06:33:41Z

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

@@ -124,6 +150,13 @@ void start() {
        if (state.compareAndSet(State.INIT, State.STARTED)) {
            scheduleWatch(0);
        }
+        if (meterRegistry != null) {
+            // emit metrics once the values are ready
+            initialValueFuture.thenAccept(latest -> {


Question) What do you think of just watching this.latest.revision() while setting -1 if not set?

I think the current approach introduces the additional complexity of maintaining an additional state.

e.g. Assuming:

val watcher = dogma.watcher()... watcher.start() watcher.close() // `initialValueFuture` is completed after the close and a gauge is registered

I also struggled with this.

Since I judged that it wouldn't become too complex, I wanted to provide as accurate metrics as possible, which is why I implemented it this way.

However, if the Central Dogma team determines that it would make maintenance more complex, I think it's fine to set it to -1.

Alternatively, Double.NaN could also be used

If you prefer precise metrics, should we use a lock to register metrics, close the watcher and remove metrics?

The lock will solve the problem that jrhee17 mentioned.

The metric registration could be placed in one method.

ReentrantLock lock = ... if (initialValueFuture.complete(newLatest)) { updateWatchMetrics(); } void updateWatchMetrics() { lock.lock(); if (isStopped()) { return; } if (this.latest == null) { // register two metrics } // update the last received time lock.unlock(); } void close() { lock.lock(); .... lock.unlock(); }

...Thinking about this topic again, the -1 seems fine.

if we don't care about -1, we can simply write ... >= 0 in PromQL.

(I prefer -1 because NaN has some quirks in handling.)

jrhee17 · 2025-02-10T06:35:35Z

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

+                "pathPattern", pathPattern,
+                // There is a possibility of using the same watcher for the same project, repo, and pathPattern.
+                // Therefore, the watcher’s hash code should be included as a tag.
+                "watcher_hash", String.valueOf(System.identityHashCode(this))


Optional) What do you think of assigning an ID instead of a hashcode on the off-chance there is a conflict? It can also help users check how many watchers there are in a JVM easily.

e.g.

private static final AtomicLong WATCHER_ID = new AtomicLong(); ... "watcher_id", String.valueOf(WATCHER_ID.incrementAndGet())

Ah, that sounds good too. If others prefer this approach, I’ll consider changing it.

How about allowing users to set the name of a Watcher? If it is absent, we may create a name automatically.

Yes, I think adding a method that allows passing a name would be a good approach.
Reference

...Personally, I feel that cases where one explicitly wants to specify a name (i.e., creating multiple watchers with the same project, repo, and path) are rare.
So, I prefer implementing it with @jrhee17 nim approach first, and if there is demand, we can add support for explicitly specifying a name.

jrhee17 · 2025-02-10T06:45:04Z

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

+                                // noinspection ConstantValue
+                                if (latestCommit.getAndSet(commit) == null) {
+                                    // emit metrics once the values are ready
+                                    meterRegistry.gauge(LATEST_COMMIT_TIME_METER_NAME, tags, latestCommit,


Question) I understand the usefulness of last commit time since it may be more convenient to check when a file was last committed as opposed to checking the revision numbers.

Having said this, this commit time may not accurate represent the current watched object since it is done in a separate IO.

What do you think of handling commit time separately by embedding commit time information in the watch response? This will involve server-side changes as well, so it may be better to handle in a separate PR.

What do you think of handling commit time separately by embedding commit time information in the watch response? This will involve server-side changes as well, so it may be better to handle in a separate PR.

Yes, I actually wanted this as well. However, since the API interface on the Central Dogma server side will also change, there seem to be many things that need to be discussed.

Isn’t it more important when the client receives a new revision for a watch?
If you think the commit time recorded in the server is meaningful, I prefer @jrhee17's approach.

Ahhh, now that you mention it, that makes sense.

It seems better to simply record the value at the time the watcher receives it, without calling the get history API.
I'll make that change. It should be much simpler.

e.g. centraldogma.client.watcher.latest.received.time

be-hase · 2025-02-14T04:08:00Z

I have fixed it.

be-hase · 2025-02-20T00:49:59Z

PTAL

jrhee17 · 2025-02-20T09:20:27Z

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

+
+        if (latestReceivedTime.getAndSet(Instant.now().getEpochSecond()) == 0) {
+            // emit metrics once the values are ready
+            meterRegistry.gauge(LATEST_RECEIVED_TIME_METER_NAME, tags, latestReceivedTime, AtomicLong::get);


Question) Will this actually register latestReceivedTime subsequently after the first registration since the Meter.Id is already registered? Would it make more sense to register the gauge once and define the update function to fetch AbstractWatcher#latestReceivedTime instead?

jrhee17 · 2025-02-20T09:40:57Z

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

@@ -124,6 +150,13 @@ void start() {
        if (state.compareAndSet(State.INIT, State.STARTED)) {
            scheduleWatch(0);
        }
+        if (meterRegistry != null) {
+            // emit metrics once the values are ready
+            initialValueFuture.thenAccept(latest -> {


Alternatively, Double.NaN could also be used

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

ikhoon · 2025-02-20T10:34:46Z

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

@@ -124,6 +150,13 @@ void start() {
        if (state.compareAndSet(State.INIT, State.STARTED)) {
            scheduleWatch(0);
        }
+        if (meterRegistry != null) {
+            // emit metrics once the values are ready
+            initialValueFuture.thenAccept(latest -> {


If you prefer precise metrics, should we use a lock to register metrics, close the watcher and remove metrics?

The lock will solve the problem that jrhee17 mentioned.

The metric registration could be placed in one method.

ReentrantLock lock = ... if (initialValueFuture.complete(newLatest)) { updateWatchMetrics(); } void updateWatchMetrics() { lock.lock(); if (isStopped()) { return; } if (this.latest == null) { // register two metrics } // update the last received time lock.unlock(); } void close() { lock.lock(); .... lock.unlock(); }

ikhoon

Thanks! 🙏🚀

ikhoon · 2025-02-21T02:30:04Z

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

@@ -247,6 +296,7 @@ private void doWatch(int numAttemptsSoFar) {
                 logger.debug("watcher noticed updated file {}/{}{}: rev={}",
                              projectName, repositoryName, pathPattern, newLatest.revision());
                 notifyListeners(newLatest);
+                 latestReceivedTimeSeconds.set(Instant.now().getEpochSecond());


nit: Should we use volatile long instead? For now, we don't get any advantage from AtomicLong.

Ah, that's right.

ikhoon · 2025-02-21T02:32:51Z

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

+                                watcher -> Optional.ofNullable(watcher.latest)
+                                                   .map(it -> it.revision().major())
+                                                   .orElse(-1));


Micro optimization) Should we use if ( != null) instead because we prefer to minimize object allocations even though it is trivial?

minwoox

Looks great. 👍 Left small suggestions. 😉

minwoox · 2025-03-04T14:14:05Z

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

+                                    if (watcher.latest == null) {
+                                        return -1;
+                                    } else {
+                                        return watcher.latest.revision().major();


nit:

Suggested change

if (watcher.latest == null) {

return -1;

} else {

return watcher.latest.revision().major();

final Latest<T> latest = watcher.latest;

if (latest == null) {

return -1;

} else {

return latest.revision().major();

Ah, I see. In that case, it’s good since it also removes the warning in the IDE.

fixed: 1d7dd3b

Yeah, it also doen't access the volatile field twice. 😉

minwoox · 2025-03-04T14:16:09Z

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

@@ -142,6 +188,12 @@ public void close() {
        if (currentWatchFuture != null && !currentWatchFuture.isDone()) {
            currentWatchFuture.cancel(false);
        }
+
+        if (meterRegistry != null) {
+            meterRegistry.remove(new Id(LATEST_REVISION_METER_NAME, tags, null, null, Type.GAUGE));


Not sure if it's a good idea to remove this gauge since it takes long to remove it when there are a lot of meters in the map.
Anyway, how about creating the gauge in start() method and remove it here?

this.gauge = Gauge.builder(LATEST_REVISION_METER_NAME, this, watcher -> { final Latest<T> latest = watcher.latest; if (latest == null) { return -1; } else { return latest.revision().major(); } }).tags(tags).register(meterRegistry); ... meterRegistry.remove(gauge);

remove this gauge since it takes long to remove

The performance regression has been resolved, so removal operations will no longer take a long time. micrometer-metrics/micrometer#5750 (comment)

Just to be sure, I checked the implementation, and it seems that using remove(Meter meter) results in the same behavior.
https://github.com/micrometer-metrics/micrometer/blob/9b1d4042579ec0691c7b235478e7fe50ce5d8eab/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java#L748

The performance regression has been resolved, so removal operations will no longer take a long time. micrometer-metrics/micrometer#5750 (comment)

Thanks for the link. I didn't know they fixed it. 😉

it seems that using remove(Meter meter) results in the same behavior.

Yeah, it simply delegates the call to the remove method that takes a meter.
I left this comment while considering a scenario where watcher.close() is called without calling watcher.start().
In that case, it would attempt to remove unregistered meters.
I understand this is unlikely in most cases, but I wanted to clarify it since modifying the code wouldn’t be difficult. 😉

minwoox

Thanks a lot for adding this nice metric, @be-hase! 😉

Report watcher metrics related to the latest revision

bda5042

be-hase mentioned this pull request Feb 6, 2025

A request to expose the latest revision and latest committed at of the watcher as metrics #1092

Open

be-hase commented Feb 6, 2025

View reviewed changes

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

support spring boot configuration

da7f001

add test

3a5e586

be-hase changed the title ~~(WIP) Report watcher metrics related to the latest revision~~ Report watcher metrics related to the latest revision Feb 6, 2025

be-hase added 3 commits February 6, 2025 20:06

comment

b605cd2

add copyright

2bdbf0a

add noMetrics test

f1a77c4

be-hase commented Feb 6, 2025

View reviewed changes

...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java Show resolved Hide resolved

be-hase marked this pull request as ready for review February 6, 2025 11:38

be-hase requested review from ikhoon, jrhee17, minwoox and trustin as code owners February 6, 2025 11:38

ikhoon reviewed Feb 7, 2025

View reviewed changes

ikhoon added the new feature label Feb 7, 2025

ikhoon added this to the 0.74.0 milestone Feb 7, 2025

jrhee17 reviewed Feb 10, 2025

View reviewed changes

be-hase added 4 commits February 12, 2025 10:33

latest received time

987dc7b

add @nullable meterRegistry() method

02432a5

fix checkstyle

ac5f3d5

use auto incremental name

935273c

jrhee17 reviewed Feb 20, 2025

View reviewed changes

ikhoon reviewed Feb 20, 2025

View reviewed changes

be-hase added 2 commits February 21, 2025 00:35

apply review feedback

d4671bf

fix checkstyke

fdc838e

ikhoon approved these changes Feb 21, 2025

View reviewed changes

jrhee17 approved these changes Feb 21, 2025

View reviewed changes

use volatile field, avoid unneeded Optional

4e97f4c

be-hase force-pushed the issue-1092 branch from 4f910d7 to 4e97f4c Compare February 21, 2025 06:44

Merge branch 'main' into issue-1092

4031055

minwoox reviewed Mar 4, 2025

View reviewed changes

minwoox modified the milestones: 0.74.0, 0.75.0 Mar 5, 2025

be-hase and others added 2 commits March 6, 2025 13:44

avoid null warning

1d7dd3b

Merge branch 'main' into issue-1092

61684c1

minwoox approved these changes Mar 6, 2025

View reviewed changes

jrhee17 mentioned this pull request Mar 6, 2025

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

Closed

		private static final String LATEST_REVISION_METER_NAME = "centraldogma.watcher.latest.revision";
		private static final String LATEST_COMMIT_TIME_METER_NAME = "centraldogma.watcher.latest.commit.time";

Report watcher metrics related to the latest revision #1094

Are you sure you want to change the base?

Report watcher metrics related to the latest revision #1094

Conversation

be-hase commented Feb 6, 2025 • edited Loading

Additional note

Tasks

CLAassistant commented Feb 6, 2025 • edited Loading

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

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

be-hase commented Feb 7, 2025

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

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 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

be-hase commented Feb 14, 2025

be-hase commented Feb 20, 2025

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

Choose a reason for hiding this comment

minwoox 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

be-hase commented Feb 6, 2025 •

edited

Loading

CLAassistant commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

ikhoon Feb 11, 2025 •

edited

Loading