[ISSUE #8354] Remove Prometheus dependency from nacos-client via Micrometer by neoLsH · Pull Request #15793 · alibaba/nacos

neoLsH · 2026-08-31T11:18:31Z

What is the purpose of the change

Fixes #8354: make nacos-client lightweight by removing the mandatory io.prometheus:simpleclient dependency, per the plugin direction discussed in the issue. Supersedes #15212, which has been stalled since 2026-05-28 with failing CI and now conflicts with develop (maintainer offered reassignment on 2026-06-26).

Brief changelog

Remove io.prometheus:simpleclient from client/pom.xml.
Add NacosClientMetricsProvider SPI (gauge / request-observation / failed-request-counter) and a NoopClientMetricsProvider default. Implementations are discovered via NacosServiceLoader (META-INF/services); without an adapter artifact on the classpath, recording is a no-op.
Refactor MetricsMonitor into a thin static facade with semantic methods (recordServiceInfoMapSize, recordListenConfigCount, observeConfigRequest, observeNamingRequest, recordNamingRequestFailed); no prometheus types remain in the client.
Fix along the way: MetricsHttpAgent previously recorded httpPost/httpDelete under the GET method label; it now records the actual method.
The existing enableClientMetrics property continues to gate all recording sites.

Users who want Prometheus metrics can supply an adapter artifact implementing the SPI (the adapter itself can live in a separate module/repository to keep the SDK lean — happy to contribute it as a follow-up).

Verifying this change

New MetricsMonitorTest (8 cases): SPI loading, delegation of all five semantic methods, no-op provider safety
Updated ClientWorkerTest, ServiceInfoHolderTest to the new static API; MetricsHttpAgentTest unaffected
Full client module tests: 1216/1216 pass; api and client-basic unaffected
mvn -pl client -am apache-rat:check checkstyle:check spotless:check spotbugs:check passes
Integration tests do not consume these client metrics (verified by repo-wide search), so no coverage regression

Fixes #8354

Make sure there is a Github issue filed for the change
Format the pull request title like [ISSUE #123] ...
Write a pull request description that is detailed enough
Write necessary unit-test to verify your logic correction
Run mvn spotless:apply and basic checks (apache-rat:check checkstyle:check spotbugs:check spotless:check) pass

…ia pluggable metrics SPI nacos-client no longer depends on io.prometheus:simpleclient. Metrics recording is delegated to a NacosClientMetricsProvider loaded via SPI, with a no-op default when no adapter is on the classpath: - add NacosClientMetricsProvider SPI and NoopClientMetricsProvider default - MetricsMonitor becomes a thin static facade with semantic methods, no prometheus types leak into the client API anymore - MetricsHttpAgent records the actual request method label (POST/DELETE were previously recorded as GET) - metrics adapter artifacts (e.g. a prometheus exporter) can implement the SPI via META-INF/services to restore metrics collection Fixes alibaba#8354 Assisted-by: Qoder

github-actions · 2026-08-31T11:18:40Z

Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io

感谢您提交的PR。 🙏
请再次查看您的PR内容，确认是否包含任何使用方式/API/配置参数的变更，如：新增API、新增配置参数、修改默认配置等操作。
如果是，请确保在提交之前，在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档（markdown格式）。

KomachiSion

Thanks for working on making nacos-client lighter. However, I don't think the current SPI-based implementation is complete enough to merge yet.

The main blocker is that this change removes the built-in Prometheus implementation and falls back to NoopClientMetricsProvider, but does not provide a production metrics adapter. Adding io.prometheus:simpleclient back to the application classpath will not restore metrics, because simpleclient does not implement or register NacosClientMetricsProvider. Existing client metrics would therefore silently disappear after upgrading.

I suggest using Micrometer instead of introducing another metrics SPI:

nacos-client already has a non-optional dependency on micrometer-core.
FailoverReactor already records client metrics through Metrics.globalRegistry.
Therefore, migrating these metrics to Micrometer adds no new base dependency or footprint.
Without a concrete registry, the global composite registry safely behaves as a no-op.
Once the application registers a PrometheusMeterRegistry, subsequent Nacos client metrics are collected automatically.

For Prometheus export, users should add and register either:

micrometer-registry-prometheus for the new Prometheus client; or
micrometer-registry-prometheus-simpleclient for the legacy client.

Adding only the underlying simpleclient artifact is not sufficient. If compatibility with plain simpleclient is required, an official bridge/adapter must be delivered together with this change rather than left as a follow-up.

Please also ensure that metrics failures can never affect client requests. In the current implementation, provider loading and recording exceptions may escape, and calls from MetricsHttpAgent are executed in finally, so a provider exception can replace a successful result or mask the original HTTP exception.

The revised implementation should include tests for:

No registry configured: client behavior remains unaffected.
A registry added after meters are created: subsequent values are collected.
PrometheusMeterRegistry.scrape() contains the expected metric names, tags, units, and buckets.
Provider/registry failures do not affect business requests.
GET, POST, and DELETE request method labels are recorded correctly.

Special attention is needed when replacing the existing Prometheus Histogram with a Micrometer Timer or DistributionSummary, because metric names, time units, and bucket definitions may change and break existing dashboards.

KomachiSion · 2026-09-01T03:20:43Z

@neoLsH Would you mind provide your dingtalk number, we would like invite you into nacos contributors group.

neoLsH · 2026-09-01T03:47:32Z

@neoLsH Would you mind provide your dingtalk number, we would like invite you into nacos contributors group.您能否提供您的钉钉号码？我们想邀请您加入 Nacos 贡献者群。

好的，我在钉钉私聊您～

… of a custom SPI Follows the review direction on alibaba#15793: drop the NacosClientMetricsProvider SPI and record everything on Metrics.globalRegistry, which nacos-client already depends on (micrometer-core) and already uses in FailoverReactor. - MetricsMonitor now registers meters on the global composite registry, so the metrics are collected as soon as the application adds a concrete registry (for example a PrometheusMeterRegistry) and are a no-op until then - metric names and tags are unchanged: gauge nacos_monitor, counter nacos_client_naming_request_failed_total; the request histogram becomes a Timer, so it is exported as nacos_client_request_seconds_* with the Prometheus default buckets kept as service level objectives - every recording path swallows Throwable, so a broken registry can never affect a client request, including the calls made from the finally block of MetricsHttpAgent - fix the request method label of MetricsHttpAgent: httpPost and httpDelete were both recorded as GET Note on units: the previous implementation passed a millisecond value to a histogram whose buckets are in seconds, so every observation fell into +Inf and the bucket data was unusable. The Timer records the elapsed time in seconds, so the buckets are now meaningful, while _sum changes from milliseconds to seconds. Assisted-by: Qoder

Adapt to the AI watch metrics added by alibaba#15795: MetricsMonitor now exposes semantic increment/decrement/read methods for the agentWatch* gauges and the nacos_client_ai_watch_events_total counter, keeping the in process values readable without a registry so AgentWatchClientMetrics and its tests keep working unchanged.

neoLsH · 2026-09-01T13:07:53Z

Thanks for the detailed review — you were right on all three points, and I have reworked the PR to use Micrometer instead of the SPI. The branch is also merged with the latest develop, which now includes the AI watch metrics from #15795.

Micrometer instead of a new SPI

MetricsMonitor now records everything on Metrics.globalRegistry, so there is no new base dependency and no second metrics system: micrometer-core is already a non-optional dependency of nacos-client and FailoverReactor already publishes through the same registry. The NacosClientMetricsProvider / NoopClientMetricsProvider pair and its META-INF/services file are gone.

Metrics come back automatically once the application registers a concrete registry (micrometer-registry-prometheus or micrometer-registry-prometheus-simpleclient), and Spring Boot users get this for free because auto-configured registries are added to the global registry by default.

Failures can never affect client requests

Every recording path in MetricsMonitor swallows Throwable now, so the calls made from the finally block of MetricsHttpAgent can neither replace a successful result nor mask the original HTTP exception. Repeated failures are logged once to avoid flooding the client log.

Compatibility, measured on a real scrape

I dumped /metrics with both registries to check the wire format rather than guessing:

Meter	Before	After
`nacos_monitor` gauge	`nacos_monitor{module,name}`	identical
`nacos_client_naming_request_failed_total`	same name	identical (Micrometer does not append a second `_total`)
`nacos_client_ai_watch_events_total`	same name	identical
`nacos_client_request` histogram	`_bucket` / `_count` / `_sum`	`nacos_client_request_seconds_{bucket,count,sum}` plus a `_seconds_max` gauge

So only the request timer is renamed. The Prometheus default bucket boundaries are kept as service level objectives, and le="+Inf" is present, so histogram_quantile() keeps working.

There is one thing worth pointing out about the units. The previous code called histogram.observe(System.currentTimeMillis() - start), i.e. it fed a millisecond value into a histogram whose buckets are in seconds, so every observation ended up in +Inf and the bucket data was never usable. The timer records the elapsed time properly, so _sum changes from milliseconds to seconds and the buckets become meaningful. Dashboards that relied on _sum/_count can be kept working with a recording rule:

- record: nacos_client_request_count
  expr: nacos_client_request_seconds_count
- record: nacos_client_request_sum
  expr: nacos_client_request_seconds_sum * 1000

I am happy to switch the timer to a DistributionSummary in milliseconds instead if you prefer to keep the old metric name at the cost of the non standard unit — it is a change in one method.

Adapting to #15795

AgentWatchClientMetrics needs inc() / dec() and, more importantly, get() on its meters, which a plain Micrometer counter cannot provide without a registry. MetricsMonitor therefore keeps the gauge values and the AI watch event counts in process and exposes semantic methods (incrementAgentWatchIntentCount(), getAgentWatchEventCount(event, result), ...). Only the ten call sites inside AgentWatchClientMetrics changed; AgentWatchClientMetricsTest and AgentWatchManagerTest pass unmodified. The read back is deliberately limited to the low cardinality AI watch meters, so the five label naming failure counter does not pay for double bookkeeping.

Tests

Covering the five cases you asked for:

no registry configured — recording is a no-op and nothing throws
a registry added after the meters exist — subsequent values are collected
PrometheusMeterRegistry.scrape() — asserts names, tags, bucket boundaries including +Inf, and timer.totalTime(MILLISECONDS) == 12 for the unit
a registry that throws — recording calls stay silent and the simulated finally path still returns the business result
GET, POST and DELETE labels in MetricsHttpAgentTest

The method label fix is included as well: httpPost and httpDelete used to be recorded as GET.

Verification: nacos-client 1320 tests pass, and mvn -pl client apache-rat:check checkstyle:check spotless:check spotbugs:check is clean.

…ethod per operation The previous revision added three methods (increment, decrement, read) for every AI watch gauge, which is duplicated code and forces MetricsMonitor to grow by three methods for each new gauge. Introduce ClientGauge, a small handle that owns the in process value, and let MetricsMonitor hand it out. This restores the shape of the original API (`getAgentWatchIntentCountMonitor().inc()`) without leaking any metrics library type, removes nine one line methods, and drops the ConcurrentHashMap and its double checked lookup, since the set of gauges is fixed at compile time. Assisted-by: Qoder

KomachiSion · 2026-09-02T07:08:42Z

Thanks for the update. The code changes look good, and the documentation follow-up does not need to block this PR.

After this PR is merged, please submit a documentation PR to the next documentation in nacos-group/nacos-group.github.io, covering:

How to enable client metrics with micrometer-registry-prometheus or micrometer-registry-prometheus-simpleclient.
How non-Spring applications should register the registry with Metrics.globalRegistry.
That adding io.prometheus:simpleclient alone is no longer sufficient.
The metric migration from nacos_client_request_{bucket,count,sum} to nacos_client_request_seconds_{bucket,count,sum}.
The _sum unit change from milliseconds to seconds, including PromQL or recording-rule migration examples for existing dashboards and alerts.

Please link the documentation PR back here once it is created.

…of prior tests The static in-process event counts are shared across the whole JVM, so AgentWatchManagerTest recording listener_callback/failed in an earlier test run leaked into the assertion expecting 0.0 and failed CI sporadically. Capture the failed-result baseline the same way the success baseline is already captured.

neoLsH · 2026-09-02T08:42:30Z

The documentation PR is up: nacos-group/nacos-group.github.io#1147.

It adds a "Java SDK Metrics" page to docs/next in both English and Chinese, covering the five points from the review:

Enabling client metrics with micrometer-registry-prometheus or micrometer-registry-prometheus-simpleclient, for both Spring Boot and plain Java applications (Metrics.globalRegistry registration).
Why a bare io.prometheus:simpleclient dependency is no longer sufficient, and that the meters silently disappear if that is all the application has.
The metric rename from nacos_client_request_{bucket,count,sum} to nacos_client_request_seconds_{bucket,count,sum}.
The _sum unit change from milliseconds to seconds, with recording-rule and PromQL migration examples for existing dashboards.
A full metric reference (names, tags, bucket boundaries) plus the enableClientMetrics switch and troubleshooting entries.

The page also notes the failure-isolation guarantee you asked about in the first review (recording failures can never affect client requests).

Separately: the latest ci run failed on MetricsMonitorTest.testAgentWatchEventCounterIsExportedAndReadableByLabels — a test-isolation issue rather than a product bug. The agent watch event counts are process-wide statics, so when AgentWatchManagerTest records a listener_callback/failed event earlier in the same JVM, the assertion expecting 0.0 for that label pair fails. I pushed 2993630, which captures the failed-result baseline the same way the success baseline was already captured; the test now passes regardless of execution order.

codecov-commenter · 2026-09-02T12:54:56Z

Codecov Report

❌ Patch coverage is 96.15385% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...m/alibaba/nacos/client/monitor/MetricsMonitor.java	93.22%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

…ests The codecov report on this PR flagged the catch branch of recordAgentWatchEvent as uncovered: ThrowingMeterRegistry only probed the timer and the naming failure counter. Extend the counter probe to the result tag and record one ai watch event while the broken registry is installed, asserting that the call is swallowed and the in-process count still increases - that read-back channel is the reason the in-process AtomicLong map exists. The gauge registration catch (registerGauge) stays uncovered on purpose: gauges are registered once during class initialization, as already documented in the test.

neoLsH · 2026-09-02T13:18:43Z

The last Integration Test run (attempt 2 on 2993630) failed during Derby schema initialization on the self-hosted runner (SQLTimeoutException: Login timeout exceeded → server never became healthy → all IT steps skipped), so it is unrelated to the change, which only touches MetricsMonitorTest.

I have pushed b20913b, which adds the missing coverage for the recordAgentWatchEvent failure path flagged by codecov: the throwing-registry test now also records one ai watch event and asserts the in-process counter still increases while the registry is broken. The gauge registration catch stays uncovered on purpose — gauges are registered once during class initialization, as documented in the test.

The workflows for the new commit are awaiting approval — could you approve them when you have a moment? Thanks.

nacos-community

This PR removes the mandatory io.prometheus:simpleclient dependency from nacos-client by re-implementing MetricsMonitor as a thin Micrometer facade, so metrics recording is a no-op when no MeterRegistry is on the classpath. The refactor is correct, thread-safe, well-tested (no-registry / late-registry / Prometheus-scrape / failure cases) and preserves Java 8 compatibility for the client module. Two compatibility notes (anchored inline) are worth capturing in the migration guide / release notes before merge; neither blocks the code change.

Suggestions

Record the Prometheus metric-name change nacos_client_request -> nacos_client_request_seconds_* (Micrometer appends the _seconds unit suffix to timers) in the release notes, since existing dashboards/alerts scraping the old name will break.
Call out the MetricsMonitor public API break (removed Prometheus-typed accessors, private constructor) in the migration guide for anyone extending the client.

Automated review by github-manager-bot

nacos-community · 2026-09-03T12:41:48Z

+
+    private static final String NACOS_MONITOR = "nacos_monitor";
+
+    private static final String NACOS_CLIENT_REQUEST = "nacos_client_request";


The nacos_client_request metric is now recorded as a Micrometer Timer, which the Prometheus registry exports as nacos_client_request_seconds_* (e.g., nacos_client_request_seconds_bucket). The previous Prometheus histogram exported nacos_client_request_* without the _seconds unit suffix, so existing dashboards and alerts that scrape this metric will break after upgrading. Document this as a breaking change in the release notes and consider whether to provide a meter filter or naming workaround to preserve the legacy name.

This one is intentional — the rename comes together with the unit fix. The old series was comparing millisecond observations against second-based bucket boundaries, so keeping the legacy name would preserve a broken metric. Migration is covered in the doc PR opened alongside this change (nacos-group/nacos-group.github.io#1147), with PromQL and recording-rule examples. A release-notes entry definitely makes sense at merge time.

nacos-community · 2026-09-03T12:41:48Z

 *
 * @author Nacos
 */
 public class MetricsMonitor {


MetricsMonitor removes the public Prometheus-typed accessor methods (getServiceInfoMapSizeMonitor, getConfigRequestMonitor, getNamingRequestFailedMonitor, etc.) and makes the constructor private. This is a source and binary compatibility break for any external code that imported this client class. The change is necessary to drop the Prometheus dependency, but it should be clearly called out in the migration guide.

Intentional as well — the handle-based ClientGauge accessors are the direction requested during the review of this PR; the old one-method-per-operation API was exactly what this refactor removed. The doc PR targets metric users (scrape / queries) rather than Java API migration, but I'm happy to add a short developer-facing note if maintainers want one.

nacos-community · 2026-09-03T12:41:48Z

+                .tag(TAG_URL, url).tag(TAG_CODE, code)
+                .serviceLevelObjectives(REQUEST_DURATION_BUCKETS).register(Metrics.globalRegistry)
+                .record(elapsedMillis, TimeUnit.MILLISECONDS);
+        } catch (Throwable t) {


Recording paths catch and swallow Throwable, including JVM Errors such as OutOfMemoryError. While this guarantees metrics can never affect client requests, it can also mask serious runtime problems. Consider catching Exception instead and letting Errors propagate, or document this deliberate resilience choice explicitly.

This is the documented resilience choice — see the class javadoc: any metrics failure is swallowed so it can never affect client requests. It's not fully silent though: the first failure logs a warn with the full stack trace, and repeats are suppressed to avoid log storms. Inside this catch there's nothing meaningful we could do for an Error either, and letting it propagate would bring back exactly the coupling this design removes. Narrowing to Exception is fine with me if maintainers prefer that.

nacos-community · 2026-09-03T12:41:48Z

+    private static void observeRequest(String module, String method, String url, String code,
+        long elapsedMillis) {
+        try {
+            Timer.builder(NACOS_CLIENT_REQUEST).tag(TAG_MODULE, module).tag(TAG_METHOD, method)


Every call to observeRequest allocates a new Timer.Builder and performs a MeterRegistry lookup. Because this is on the request hot path, consider caching frequently-used timers (and similarly counters) by their tag tuple if profiling shows allocation pressure.

Deferred until profiling shows a need — the registry already caches meters by id, so a repeated register returns the existing instance and the per-call cost is just the builder allocation plus the lookup. Client request volume is far below where that becomes measurable. If it ever shows up in a profile, caching timers by tag tuple would be a small follow-up.

The ai watch counter assertions added in b20913b exceeded the line length limit, which failed the spotless check in CI. Reformat them with 'mvn spotless:apply'; no logic change.

neoLsH · 2026-09-03T13:08:18Z

The ci run on b20913b failed on spotless format violations (the ai watch counter assertions added in that commit exceeded the line length limit) — not a test failure; the Integration Test suite itself passed this time.

I have pushed 14d2d07, which only reformats those lines via mvn spotless:apply — no logic change. Verified locally: mvn -pl client spotless:check passes and MetricsMonitorTest stays green (6/6).

The new run is awaiting workflow approval again — thanks in advance.

nacos-community

Follow-up on the new commit: I compared (previous head) with 14d2d079c — the incremental change is limited to client/src/test/java/com/alibaba/nacos/client/monitor/MetricsMonitorTest.java (+6/−3) and consists solely of line wrapping applied by mvn spotless:apply (no logic change), matching the description above. The CI failure on the previous head was a spotless formatting violation, not a test failure.

My earlier review comments on this PR remain applicable; no new findings from this formatting commit.

Automated review by github-manager-bot

nacos-community

Thanks for the detailed responses, @neoLsH — I verified against 14d2d079c and accept all four explanations:

Metric rename (MetricsMonitor.java:61) — Understood: the rename is tied to the unit fix (the old series compared millisecond observations against second-based buckets), and migration is covered by the doc PR nacos-group/nacos-group.github.io#1147. Agreed that a release-notes entry at merge time makes sense.
Timer lookup cost (MetricsMonitor.java:199) — Deferral accepted. The registry returns the cached meter instance on repeated registration, so the per-call cost is bounded; revisiting via a tag-tuple cache can wait until profiling shows pressure.
catch (Throwable) (MetricsMonitor.java:203) — Confirmed this is the documented resilience choice (class javadoc, line 45), with the first failure logged as a warn including the stack trace and repeats suppressed via the METRICS_FAILURE_LOGGED guard. Whether to narrow to Exception is left to the maintainers.
Removed Prometheus-typed accessors (MetricsMonitor.java:49) — Acknowledged: the handle-based ClientGauge accessors are the agreed direction of this refactor. A short developer-facing note is at the maintainers' discretion.

No further objections from my side on these points. Remaining decision items (narrowing the catch to Exception, developer-facing migration note, release-notes entry) are deferred to the maintainers.

Automated review by github-manager-bot

KomachiSion requested changes Sep 1, 2026

View reviewed changes

neoLsH added 2 commits September 1, 2026 20:58

KomachiSion changed the title ~~[ISSUE #8354] Remove prometheus dependency from nacos-client via pluggable metrics SPI~~ [ISSUE #8354] Remove Prometheus dependency from nacos-client via Micrometer Sep 2, 2026

neoLsH mentioned this pull request Sep 2, 2026

docs: add Java SDK client metrics guide for the Micrometer migration (follow-up to alibaba/nacos#15793) nacos-group/nacos-group.github.io#1147

Open

6 tasks

nacos-community mentioned this pull request Sep 3, 2026

[ISSUE #8354] Make prometheus client optional in nacos-client SDK #15212

Open

nacos-community reviewed Sep 3, 2026

View reviewed changes

[ISSUE alibaba#8354] Fix spotless format violations in metrics tests

14d2d07

The ai watch counter assertions added in b20913b exceeded the line length limit, which failed the spotless check in CI. Reformat them with 'mvn spotless:apply'; no logic change.

nacos-community reviewed Sep 3, 2026

View reviewed changes


		private static final String NACOS_MONITOR = "nacos_monitor";

		private static final String NACOS_CLIENT_REQUEST = "nacos_client_request";

Uh oh!

Conversation

neoLsH commented Aug 31, 2026

What is the purpose of the change

Brief changelog

Verifying this change

Uh oh!

github-actions Bot commented Aug 31, 2026

Uh oh!

KomachiSion left a comment

Choose a reason for hiding this comment

Uh oh!

KomachiSion commented Sep 1, 2026

Uh oh!

neoLsH commented Sep 1, 2026

Uh oh!

neoLsH commented Sep 1, 2026

Micrometer instead of a new SPI

Failures can never affect client requests

Compatibility, measured on a real scrape

Adapting to #15795

Tests

Uh oh!

KomachiSion commented Sep 2, 2026

Uh oh!

neoLsH commented Sep 2, 2026

Uh oh!

codecov-commenter commented Sep 2, 2026

Codecov Report

Uh oh!

neoLsH commented Sep 2, 2026

Uh oh!

nacos-community left a comment

Choose a reason for hiding this comment

Suggestions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neoLsH commented Sep 3, 2026

Uh oh!

nacos-community left a comment

Choose a reason for hiding this comment

Uh oh!

nacos-community left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants