You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "codelipenghui (via GitHub)" <gi...@apache.org> on 2025/09/10 18:21:54 UTC
[PR] [feat][broker] Add broker-level metrics for non-recoverable data skips [pulsar]
codelipenghui opened a new pull request, #24726:
URL: https://github.com/apache/pulsar/pull/24726
## Summary
This PR implements broker-level metrics for tracking non-recoverable data skips in Apache Pulsar, providing operational visibility for data loss events.
### Key Features Added
- **NonRecoverableDataMetricsCallback Interface**: New callback interface in managed-ledger module to avoid cross-module dependencies
- **ManagedLedger Integration**: Updated `ManagedLedgerImpl.skipNonRecoverableLedger()` to trigger callback when ledgers are skipped
- **ManagedCursor Integration**: Updated `ManagedCursorImpl.skipNonRecoverableEntries()` to count and report skipped entries
- **BrokerService Configuration**: Automatic callback setup during managed ledger creation for topics
- **Dual Metrics Support**: Both Prometheus and OpenTelemetry metrics implementation
### New Metrics
1. **`pulsar.broker.non.recoverable.ledgers.skipped.count`** - Tracks the number of entire ledgers skipped due to non-recoverable issues
2. **`pulsar.broker.non.recoverable.entries.skipped.count`** - Tracks the number of individual entries skipped due to non-recoverable issues
### Design Decisions
- **Always Available**: Metrics work regardless of `ManagedLedgerInterceptor` configuration state
- **Callback Pattern**: Uses callback interface to avoid introducing dependencies between managed-ledger and broker modules
- **Automatic Setup**: Callbacks are automatically configured by `BrokerService` during topic creation
- **Null-Safe**: All implementations handle null callback gracefully
### Testing Coverage
- **7 Unit Tests**: Comprehensive testing of callback integration in `NonRecoverableDataCallbackTest`
- **3 Integration Tests**: End-to-end validation including real topic/subscription creation in `OpenTelemetryBrokerOperabilityStatsTest`
- **Checkstyle Clean**: All code follows Apache Pulsar coding standards
### Implementation Details
The implementation uses a callback pattern where:
1. `BrokerService` sets up the `NonRecoverableDataMetricsCallback` during managed ledger configuration
2. `ManagedLedgerImpl` calls the callback when `skipNonRecoverableLedger()` is invoked
3. `ManagedCursorImpl` calls the callback when `skipNonRecoverableEntries()` is invoked with actual entry counts
4. The callback updates `BrokerOperabilityMetrics` which maintains both Prometheus and OpenTelemetry counters
## Test plan
- [x] Unit tests pass (7/7 in managed-ledger module)
- [x] Integration tests pass (3/3 in broker module)
- [x] Checkstyle validation passes (0 violations)
- [x] End-to-end testing with real topics, subscriptions, and message positions
- [x] Callback integration verified through actual broker service topic creation
🤖 Generated with [Claude Code](https://claude.ai/code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][broker] PIP-441:Add broker-level metrics for non-recoverable data skips [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #24726:
URL: https://github.com/apache/pulsar/pull/24726#issuecomment-3276060228
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][broker] PIP-441:Add broker-level metrics for non-recoverable data skips [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24726:
URL: https://github.com/apache/pulsar/pull/24726#issuecomment-3282876094
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/24726?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.30435%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 74.11%. Comparing base ([`a2b69cc`](https://app.codecov.io/gh/apache/pulsar/commit/a2b69ccbc626192c544e07276bbfaa4195e0b6b5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a6f6905`](https://app.codecov.io/gh/apache/pulsar/commit/a6f69058637dc7612ced51429122aa26561a496d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24726?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fstats%2FBrokerOperabilityMetrics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | 93.93% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/bookkeeper/mledger/impl/ManagedCursorImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FManagedCursorImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JJbXBsLmphdmE=) | 75.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rg/apache/pulsar/broker/service/BrokerService.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FBrokerService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | 87.50% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #24726 +/- ##
============================================
- Coverage 74.17% 74.11% -0.06%
+ Complexity 33470 33117 -353
============================================
Files 1895 1896 +1
Lines 148029 148158 +129
Branches 17142 17168 +26
============================================
+ Hits 109796 109807 +11
- Misses 29479 29570 +91
- Partials 8754 8781 +27
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/24726/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/24726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.34% <58.69%> (+0.10%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/24726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.65% <50.00%> (-0.01%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/24726/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.65% <91.30%> (-0.04%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24726?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...apache/bookkeeper/mledger/ManagedLedgerConfig.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2FManagedLedgerConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9NYW5hZ2VkTGVkZ2VyQ29uZmlnLmphdmE=) | `96.57% <ø> (ø)` | |
| [...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FManagedLedgerImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRMZWRnZXJJbXBsLmphdmE=) | `80.68% <100.00%> (-0.25%)` | :arrow_down: |
| [...che/bookkeeper/mledger/impl/ManagedCursorImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FManagedCursorImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JJbXBsLmphdmE=) | `77.83% <75.00%> (-0.57%)` | :arrow_down: |
| [...rg/apache/pulsar/broker/service/BrokerService.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FBrokerService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | `83.57% <87.50%> (+0.15%)` | :arrow_up: |
| [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://app.codecov.io/gh/apache/pulsar/pull/24726?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fstats%2FBrokerOperabilityMetrics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | `93.70% <93.93%> (+0.08%)` | :arrow_up: |
... and [95 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/24726/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][broker] PIP-441:Add broker-level metrics for non-recoverable data skips [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24726:
URL: https://github.com/apache/pulsar/pull/24726#issuecomment-3276052948
@codelipenghui Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][broker] PIP-441:Add broker-level metrics for non-recoverable data skips [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #24726:
URL: https://github.com/apache/pulsar/pull/24726#discussion_r2371552133
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1808,6 +1808,25 @@ public void createPersistentTopic0(final String topic, boolean createIfMissing,
managedLedgerConfig.setManagedLedgerInterceptor(
new ManagedLedgerInterceptorImpl(interceptors, brokerEntryPayloadProcessors));
}
+
+ // Set non-recoverable data metrics callback
+ if (pulsarStats.getBrokerOperabilityMetrics() != null) {
+ managedLedgerConfig.setNonRecoverableDataMetricsCallback(
+ new org.apache.bookkeeper.mledger.NonRecoverableDataMetricsCallback() {
+ @Override
+ public void onSkipNonRecoverableLedger(long ledgerId) {
+ pulsarStats.getBrokerOperabilityMetrics().recordNonRecoverableLedgerSkipped();
+ }
+
+ @Override
+ public void onSkipNonRecoverableEntries(long entryCount) {
+ for (long i = 0; i < entryCount; i++) {
+ pulsarStats.getBrokerOperabilityMetrics().recordNonRecoverableEntriesSkipped();
Review Comment:
addressed
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1808,6 +1808,25 @@ public void createPersistentTopic0(final String topic, boolean createIfMissing,
managedLedgerConfig.setManagedLedgerInterceptor(
new ManagedLedgerInterceptorImpl(interceptors, brokerEntryPayloadProcessors));
}
+
+ // Set non-recoverable data metrics callback
+ if (pulsarStats.getBrokerOperabilityMetrics() != null) {
+ managedLedgerConfig.setNonRecoverableDataMetricsCallback(
+ new org.apache.bookkeeper.mledger.NonRecoverableDataMetricsCallback() {
+ @Override
+ public void onSkipNonRecoverableLedger(long ledgerId) {
+ pulsarStats.getBrokerOperabilityMetrics().recordNonRecoverableLedgerSkipped();
+ }
+
+ @Override
+ public void onSkipNonRecoverableEntries(long entryCount) {
+ for (long i = 0; i < entryCount; i++) {
+ pulsarStats.getBrokerOperabilityMetrics().recordNonRecoverableEntriesSkipped();
Review Comment:
please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][broker] PIP-441:Add broker-level metrics for non-recoverable data skips [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #24726:
URL: https://github.com/apache/pulsar/pull/24726
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org