Feature: Support OpenTelemetry API for Apollo Monitor custom metrics reporting#143
Feature: Support OpenTelemetry API for Apollo Monitor custom metrics reporting#143teaho2015 wants to merge 2 commits into
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
📝 WalkthroughWalkthroughAdded an OpenTelemetry client metrics plugin module with SPI registration, counter and gauge export support, tag attribute caching, status reporting, and unit tests covering initialized and uninitialized behavior. ChangesOpenTelemetry metrics integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ApolloClientMetricsExporter
participant OpenTelemetryApolloClientMetricsExporter
participant GlobalOpenTelemetry
participant Meter
ApolloClientMetricsExporter->>OpenTelemetryApolloClientMetricsExporter: initialize exporter
OpenTelemetryApolloClientMetricsExporter->>GlobalOpenTelemetry: get()
GlobalOpenTelemetry-->>OpenTelemetryApolloClientMetricsExporter: OpenTelemetry instance
OpenTelemetryApolloClientMetricsExporter->>Meter: build counter or observable gauge
OpenTelemetryApolloClientMetricsExporter->>Meter: record metric values
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
============================================
+ Coverage 68.68% 71.07% +2.39%
- Complexity 1503 1656 +153
============================================
Files 212 225 +13
Lines 6396 6798 +402
Branches 647 685 +38
============================================
+ Hits 4393 4832 +439
+ Misses 1673 1612 -61
- Partials 330 354 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporterTest.java (1)
75-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for null tags with initialized meter.
The counter and gauge tests verify builder interactions but don't test null tags when the meter is initialized. This is the exact scenario where
registerOrUpdateGaugeSamplewill NPE (viagetGaugeKey→createCacheKey). Adding this test would have caught the bug flagged in the implementation.Additionally,
testRegisterOrUpdateCounterSampleverifiescounterBuilder.build()but doesn't verifycounter.add()was called with the expected value and attributes.🧪 Suggested additional test
`@Test`(expected = NullPointerException.class) public void testRegisterOrUpdateGaugeSampleWithNullTags() { // This should NOT throw after the fix; before the fix it throws NPE // After fixing createCacheKey to handle null, update this test to assert no exception exporter.registerOrUpdateGaugeSample("test_gauge", null, 1.0); } `@Test` public void testRegisterOrUpdateCounterSampleWithNullTags() { io.opentelemetry.api.metrics.LongCounterBuilder counterBuilder = mock(io.opentelemetry.api.metrics.LongCounterBuilder.class); io.opentelemetry.api.metrics.LongCounter counter = mock(io.opentelemetry.api.metrics.LongCounter.class); when(meter.counterBuilder("null_tags_counter")).thenReturn(counterBuilder); when(counterBuilder.setDescription(anyString())).thenReturn(counterBuilder); when(counterBuilder.setUnit(anyString())).thenReturn(counterBuilder); when(counterBuilder.build()).thenReturn(counter); exporter.registerOrUpdateCounterSample("null_tags_counter", null, 1.0); verify(counter).add(eq(1L), any()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporterTest.java` around lines 75 - 121, Extend testRegisterOrUpdateGaugeSample and testRegisterOrUpdateCounterSample to cover initialized-meter calls with null tags, asserting they complete without throwing and that the counter invokes add with the expected value and attributes. In testRegisterOrUpdateCounterSample, retain the builder verifications and add verification of counter.add; configure the mocks using the existing meter and builder setup.apollo-plugin/apollo-plugin-client-opentelemetry/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporter.java (1)
113-115: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse
AtomicReferenceinstead of allocating a new one per update.
gaugeValueMap.put(gaugeKey, new AtomicReference<>(value))creates a newAtomicReferenceobject on every gauge update. The callback increateGaugereads from the map each time, so functionally this works, but it produces unnecessary garbage. UsecomputeIfAbsent+setto reuse the existing holder.♻️ Proposed refactor
- gaugeValueMap.put(gaugeKey, new AtomicReference<>(value)); + gaugeValueMap.computeIfAbsent(gaugeKey, k -> new AtomicReference<>()).set(value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apollo-plugin/apollo-plugin-client-opentelemetry/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporter.java` around lines 113 - 115, Update the gauge storage logic in the exporter method containing gaugeKey and gaugeValueMap so it reuses the existing AtomicReference: obtain the holder with computeIfAbsent and set the new value on it, rather than creating a new AtomicReference on every update.apollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/SimpleOpenTelemetryApolloClientMetricsExporterTest.java (1)
54-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUninitialized-behavior tests don't assert anything meaningful.
Tests
testRegisterOrUpdateCounterSampleWithoutInit,testRegisterOrUpdateGaugeSampleWithoutInit,testEmptyTags, andtestNullTagsall follow the pattern: call method in try-catch, pass whether or not an exception is thrown. This means they pass even if the methods throw unexpected exceptions or silently corrupt state.Since
meteris null (nodoInit()), the methods should return early with a warning. The tests should verify this explicitly rather than accepting any outcome.🧪 Suggested improvement
`@Test` public void testRegisterOrUpdateCounterSampleWithoutInit() { - String name = "test_counter"; - Map<String, String> tags = new HashMap<>(); - tags.put("namespace", "application"); - - try { - exporter.registerOrUpdateCounterSample(name, tags, 1.0); - assertTrue(true); - } catch (Exception e) { - // It's okay if it throws exception when not initialized - } + String name = "test_counter"; + Map<String, String> tags = new HashMap<>(); + tags.put("namespace", "application"); + + exporter.registerOrUpdateCounterSample(name, tags, 1.0); + + // Verify no metrics were registered + String response = exporter.response(); + assertTrue(response.contains("Counters: 0")); + assertTrue(response.contains("Meter: not initialized")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/SimpleOpenTelemetryApolloClientMetricsExporterTest.java` around lines 54 - 113, Update testRegisterOrUpdateCounterSampleWithoutInit, testRegisterOrUpdateGaugeSampleWithoutInit, testEmptyTags, and testNullTags to assert the exporter’s expected uninitialized behavior: with meter unset, each operation should return normally without throwing and leave state unchanged. Remove broad try-catch blocks and unconditional assertions, and verify the relevant warning/early-return outcome using the test’s available observability mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apollo-plugin/apollo-plugin-client-opentelemetry/pom.xml`:
- Around line 40-43: Update the opentelemetry-api dependency version from 1.35.0
to at least 1.62.0, preferably the stable 1.63.0 release; if the dependency
version is shared across modules, define it in the parent dependencyManagement
and reuse it here.
In
`@apollo-plugin/apollo-plugin-client-opentelemetry/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporter.java`:
- Around line 106-122: Update registerOrUpdateGaugeSample to wrap gauge value
storage and registration in the same try-catch pattern used by
registerOrUpdateCounterSample, logging failures without propagating exceptions.
Also update createCacheKey to handle null tags before iterating entrySet,
preserving the existing key generation for non-null tags.
---
Nitpick comments:
In
`@apollo-plugin/apollo-plugin-client-opentelemetry/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporter.java`:
- Around line 113-115: Update the gauge storage logic in the exporter method
containing gaugeKey and gaugeValueMap so it reuses the existing AtomicReference:
obtain the holder with computeIfAbsent and set the new value on it, rather than
creating a new AtomicReference on every update.
In
`@apollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporterTest.java`:
- Around line 75-121: Extend testRegisterOrUpdateGaugeSample and
testRegisterOrUpdateCounterSample to cover initialized-meter calls with null
tags, asserting they complete without throwing and that the counter invokes add
with the expected value and attributes. In testRegisterOrUpdateCounterSample,
retain the builder verifications and add verification of counter.add; configure
the mocks using the existing meter and builder setup.
In
`@apollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/SimpleOpenTelemetryApolloClientMetricsExporterTest.java`:
- Around line 54-113: Update testRegisterOrUpdateCounterSampleWithoutInit,
testRegisterOrUpdateGaugeSampleWithoutInit, testEmptyTags, and testNullTags to
assert the exporter’s expected uninitialized behavior: with meter unset, each
operation should return normally without throwing and leave state unchanged.
Remove broad try-catch blocks and unconditional assertions, and verify the
relevant warning/early-return outcome using the test’s available observability
mechanism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da8f9cbb-981f-48c0-97b4-c598df2d29f8
📒 Files selected for processing (6)
apollo-plugin/apollo-plugin-client-opentelemetry/pom.xmlapollo-plugin/apollo-plugin-client-opentelemetry/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporter.javaapollo-plugin/apollo-plugin-client-opentelemetry/src/main/resources/META-INF/services/com.ctrip.framework.apollo.monitor.internal.exporter.ApolloClientMetricsExporterapollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporterTest.javaapollo-plugin/apollo-plugin-client-opentelemetry/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/SimpleOpenTelemetryApolloClientMetricsExporterTest.javaapollo-plugin/pom.xml
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-api</artifactId> | ||
| <version>1.35.0</version> | ||
| </dependency> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of io.opentelemetry:opentelemetry-api, and are there any security advisories for version 1.35.0?
💡 Result:
As of July 12, 2026, the latest stable version of io.opentelemetry:opentelemetry-api is 1.63.0 [1][2]. Regarding version 1.35.0, it is affected by a security advisory: GHSA-rcgg-9c38-7xpx: Unbounded Memory Allocation in W3C Baggage Propagation. This vulnerability, identified as CVE-2026-45292, affects versions up to and including 1.61.0 [3]. Because version 1.35.0 is included in this range, it is considered vulnerable to this issue [3][4]. The vulnerability involves the lack of limits on W3C Baggage header size and entry counts, which can lead to excessive memory allocation and CPU consumption [3]. This was addressed in version 1.62.0 [3].
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-java?tab=readme-ov-file
- 2: https://javadoc.io/doc/io.opentelemetry/opentelemetry-api/latest/index.html
- 3: GHSA-rcgg-9c38-7xpx
- 4: https://deps.dev/maven/io.opentelemetry%3Aopentelemetry-api/1.35.0
Update opentelemetry-api to a fixed release — 1.35.0 is affected by GHSA-rcgg-9c38-7xpx (unbounded memory allocation in W3C baggage propagation); upgrade to at least 1.62.0 (latest stable is 1.63.0). If this dependency is shared elsewhere, move the version into the parent dependencyManagement section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apollo-plugin/apollo-plugin-client-opentelemetry/pom.xml` around lines 40 -
43, Update the opentelemetry-api dependency version from 1.35.0 to at least
1.62.0, preferably the stable 1.63.0 release; if the dependency version is
shared across modules, define it in the parent dependencyManagement and reuse it
here.
| @Override | ||
| public void registerOrUpdateGaugeSample(String name, Map<String, String> tags, double value) { | ||
| if (meter == null) { | ||
| logger.warn("OpenTelemetry meter not initialized, skipping gauge registration for '{}'", name); | ||
| return; | ||
| } | ||
|
|
||
| // Store the gauge value | ||
| String gaugeKey = getGaugeKey(name, tags); | ||
| gaugeValueMap.put(gaugeKey, new AtomicReference<>(value)); | ||
|
|
||
| // Register gauge if not already registered | ||
| gaugeMap.computeIfAbsent(gaugeKey, key -> createGauge(name, tags, gaugeKey)); | ||
|
|
||
| logger.debug("Updated OpenTelemetry gauge '{}' with value: {}, tags: {}", | ||
| name, value, tags); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
registerOrUpdateGaugeSample lacks error handling and will NPE on null tags.
Two issues:
-
Inconsistent error handling:
registerOrUpdateCounterSamplewraps all logic in try-catch, butregisterOrUpdateGaugeSampledoes not. Ifmeter.gaugeBuilder(name)orbuildWithCallbackthrows, the exception propagates unhandled to the caller, potentially crashing the monitoring thread. -
NPE on null tags:
getGaugeKey(name, tags)→createCacheKey(tags)callstags.entrySet()without a null check. When the meter is initialized andtagsis null, this throws NPE. Note thatgetOrCreateAttributeshandles null tags, but it's called insidecreateGauge— aftergetGaugeKeyhas already thrown.
🐛 Proposed fix: add try-catch and null-guard in createCacheKey
`@Override`
public void registerOrUpdateGaugeSample(String name, Map<String, String> tags, double value) {
- if (meter == null) {
- logger.warn("OpenTelemetry meter not initialized, skipping gauge registration for '{}'", name);
- return;
- }
-
- // Store the gauge value
- String gaugeKey = getGaugeKey(name, tags);
- gaugeValueMap.put(gaugeKey, new AtomicReference<>(value));
-
- // Register gauge if not already registered
- gaugeMap.computeIfAbsent(gaugeKey, key -> createGauge(name, tags, gaugeKey));
-
- logger.debug("Updated OpenTelemetry gauge '{}' with value: {}, tags: {}",
- name, value, tags);
+ try {
+ if (meter == null) {
+ logger.warn("OpenTelemetry meter not initialized, skipping gauge registration for '{}'", name);
+ return;
+ }
+
+ // Store the gauge value
+ String gaugeKey = getGaugeKey(name, tags);
+ gaugeValueMap.computeIfAbsent(gaugeKey, k -> new AtomicReference<>()).set(value);
+
+ // Register gauge if not already registered
+ gaugeMap.computeIfAbsent(gaugeKey, key -> createGauge(name, tags, gaugeKey));
+
+ logger.debug("Updated OpenTelemetry gauge '{}' with value: {}, tags: {}",
+ name, value, tags);
+ } catch (Exception e) {
+ logger.error("Failed to register or update OpenTelemetry gauge '{}'", name, e);
+ }
}And add a null guard in createCacheKey:
private String createCacheKey(Map<String, String> tags) {
+ if (tags == null || tags.isEmpty()) {
+ return "";
+ }
// Sort keys to ensure consistent cache key
return tags.entrySet().stream()
.sorted(Map.Entry.comparingByKey())
.map(entry -> entry.getKey() + "=" + entry.getValue())
.reduce((a, b) -> a + ";" + b)
.orElse("");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void registerOrUpdateGaugeSample(String name, Map<String, String> tags, double value) { | |
| if (meter == null) { | |
| logger.warn("OpenTelemetry meter not initialized, skipping gauge registration for '{}'", name); | |
| return; | |
| } | |
| // Store the gauge value | |
| String gaugeKey = getGaugeKey(name, tags); | |
| gaugeValueMap.put(gaugeKey, new AtomicReference<>(value)); | |
| // Register gauge if not already registered | |
| gaugeMap.computeIfAbsent(gaugeKey, key -> createGauge(name, tags, gaugeKey)); | |
| logger.debug("Updated OpenTelemetry gauge '{}' with value: {}, tags: {}", | |
| name, value, tags); | |
| } | |
| `@Override` | |
| public void registerOrUpdateGaugeSample(String name, Map<String, String> tags, double value) { | |
| try { | |
| if (meter == null) { | |
| logger.warn("OpenTelemetry meter not initialized, skipping gauge registration for '{}'", name); | |
| return; | |
| } | |
| // Store the gauge value | |
| String gaugeKey = getGaugeKey(name, tags); | |
| gaugeValueMap.computeIfAbsent(gaugeKey, k -> new AtomicReference<>()).set(value); | |
| // Register gauge if not already registered | |
| gaugeMap.computeIfAbsent(gaugeKey, key -> createGauge(name, tags, gaugeKey)); | |
| logger.debug("Updated OpenTelemetry gauge '{}' with value: {}, tags: {}", | |
| name, value, tags); | |
| } catch (Exception e) { | |
| logger.error("Failed to register or update OpenTelemetry gauge '{}'", name, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apollo-plugin/apollo-plugin-client-opentelemetry/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/OpenTelemetryApolloClientMetricsExporter.java`
around lines 106 - 122, Update registerOrUpdateGaugeSample to wrap gauge value
storage and registration in the same try-catch pattern used by
registerOrUpdateCounterSample, logging failures without propagating exceptions.
Also update createCacheKey to handle null tags before iterating entrySet,
preserving the existing key generation for non-null tags.
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
Thanks for adding the OpenTelemetry metrics exporter. The SPI wiring and the existing compatibility checks look good, but there are still a few blocking issues on the current head:
-
opentelemetry-api:1.35.0is affected by GHSA-rcgg-9c38-7xpx. Please upgrade to a patched supported version (at least 1.62.0; 1.64.0 is the current stable release at review time). -
registerOrUpdateCounterSamplecasts the API'sdoubleincrement tolong. This silently exports0.5as0. Please use a double-valued OpenTelemetry counter and add a regression test covering a fractional increment and its attributes. -
Please address the current gauge null-tags/error-handling issue and replace the broad try/catch tests with assertions that verify the actual counter value, attributes, gauge callback output, and uninitialized behavior.
-
Since this is a user-visible feature, please update
CHANGES.mdand add minimal usage documentation covering the plugin dependency,apollo.client.monitor.enabled,apollo.client.monitor.external.type=opentelemetry, and the requirement to initialize the OpenTelemetry SDK before Apollo monitoring starts.
Before merge, please also sign the CLA, remove the trailing whitespace reported by git diff --check, and squash the branch to one Conventional Commit.
2 participants
Description
Add OpenTelemetry API integration to Apollo Monitor, enabling users to report Apollo custom monitoring metrics via OpenTelemetry metrics API.
Motivation
Currently Apollo Monitor only supports built-in monitoring reporters. This feature allows observability platforms compatible with OpenTelemetry to consume Apollo monitoring metrics natively, unifying metrics collection in cloud-native environments.
Changes
Testing
Summary by CodeRabbit
New Features
Tests