Repository navigation
[feature](metrics) Add the changelog monitor to the Master FE - #69848
Open
morningman wants to merge 3 commits into
Open
morningman wants to merge 3 commits into
morningman wants to merge 3 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Table streams and IVM materialized views have no
metrics. How much changelog the tables keep shows only in `SHOW DATA`,
and nothing lets an alert tell whether an IVM materialized view is
fresh or a stream is about to fall behind its changelog TTL. Those
signals need per-partition state that costs a walk over the metadata,
and in cloud mode a MetaService call, so they cannot be computed while
`/metrics` is scraped: `MetricRepo.getMetric` renders the whole
response under one lock.
This adds the framework the coming signals are rendered from, and the
first series on it.
- `ChangelogIvmMonitor`, a daemon of the Master FE, samples every
`changelog_ivm_monitor_interval_seconds`, builds an immutable
`ChangelogIvmMonitorSnapshot` and replaces the previous one at once.
A round that fails keeps the previous snapshot and counts the
failure.
- `MetricVisitor.visitChangelogIvmMonitor`, beside
`visitCloudTableStats`, renders the series from the latest snapshot.
Only the Master exports them. Another FE exports none rather than 0,
so a `min` or `max` across the FEs is not dragged to 0, and a new
Master exports none of the sampled series until its first round
completes. The JSON and core outputs of `/metrics` render nothing.
- `doris_fe_changelog_retained_bytes{scope}` is the changelog kept by
all the tables, summed from the table statistics that `SHOW DATA`
reports as `ChangelogSize`: `single_replica` counts one replica of
each tablet and `all_replicas` all of them. Cloud mode exports
`single_replica` only, as a tablet keeps one copy in the shared
storage.
- `doris_fe_changelog_ivm_monitor_last_sample_timestamp_seconds`,
`doris_fe_changelog_ivm_monitor_sample_duration_seconds` and
`doris_fe_changelog_ivm_monitor_sample_failures_total` are the health
of the monitor, so that an alert tells stale data (the latest round
long ago, or the series absent) from a healthy state.
Two FE configs, mutable, Master-only and EXPERIMENTAL like the other
table stream configs: `enable_changelog_ivm_monitor` (true) takes
effect with `enable_feature_changelog` only, so a cluster without the
changelog feature exports nothing new; `changelog_ivm_monitor_interval_seconds`
(15) is checked every second, so a change applies at once.
The state of the streams and of the IVM materialized views, their
series of their own and the quota on those come with follow-up PRs on
this framework.
### Release note
With `enable_feature_changelog` on, the Master FE exports
`doris_fe_changelog_retained_bytes{scope}` and the health of its
sampling, `doris_fe_changelog_ivm_monitor_last_sample_timestamp_seconds`,
`doris_fe_changelog_ivm_monitor_sample_duration_seconds` and
`doris_fe_changelog_ivm_monitor_sample_failures_total`. New FE configs
`enable_changelog_ivm_monitor` and `changelog_ivm_monitor_interval_seconds`.
### Check List (For Author)
- Test: Unit Test, Regression test, Manual test
- `ChangelogIvmMonitorTest`: a round sums the changelog that
`SHOW DATA` reads from the table statistics; a failed round keeps
the previous snapshot and counts the failure; rounds follow the
interval, a shorter interval applies at the next check, and a
disabled monitor samples nothing and drops its snapshot.
- `ChangelogIvmMonitorMetricsTest`: the exact text of the series,
also through `MetricRepo.getMetric`; only the failure counter
before the first round; nothing on a Follower or with the monitor
or the changelog feature disabled; `single_replica` only in cloud
mode; nothing in the JSON and core outputs.
- `changelog_p0/test_changelog_ivm_monitor_metrics` (run locally,
not in cloud mode): on the `/metrics` of the Master,
`all_replicas` (`single_replica` in cloud mode) equals the
`ChangelogSize` total of
`SHOW DATA PROPERTIES ("entire_warehouse" = "true")`; the health
series are present and fresh; no other FE exports them; nothing
once the monitor is disabled.
- Manual: `promtool check metrics` on the `/metrics` of the Master
reports no parsing error and no lint finding on the new series.
- Behavior changed: Yes. With `enable_feature_changelog` on, the
`/metrics` of the Master FE has the four new families, and a Master
daemon sums the table statistics every 15 seconds. Nothing changes
otherwise.
- Does this need documentation: Yes, together with the rest of the
changelog and IVM monitoring, in a follow-up doc PR.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed PR 69848 at the pinned head 0b70734 against base 2a4191c. The new Master-only monitor, immutable snapshot publication, failure retention/counter, Prometheus-only rendering, and cloud/shared-nothing changelog semantics match the stated feature goal and SHOW DATA aggregation. Catalog/database collections are concurrent-map snapshots, table statistics are volatile whole-object replacements, and no new lock, RPC, journal, persistence, thrift/protobuf, or FE role-transfer defect was found. All MetricVisitor implementations and the inspected unit/regression paths are covered; the tests were not executed because this review is static-only under the review contract.
One verified P2 finding is included inline: the enable gate is checked only before sampling and snapshots are published unconditionally. Disabling and re-enabling within the one-second polling window, or disabling during a long catalog walk, can expose a snapshot from the disabled period after re-enable. No existing inline findings apply, and no additional user review focus was supplied.
All three review lanes completed their assigned coverage and independently rechecked the candidate. Every other risk was dismissed with evidence, so this review converged in round 1 with no remaining unresolved candidate.
Review converged after 1 round(s).
|
|
||
| @Override | ||
| protected void runAfterCatalogReady() { | ||
| if (!isEnabled()) { |
Contributor
There was a problem hiding this comment.
[P2] Clear or reject snapshots across monitor disable/re-enable transitions. runAfterCatalogReady() checks isEnabled() before sample(), but sample() publishes unconditionally. If enable_changelog_ivm_monitor is set false during a catalog walk, or toggled false/true within the one-second polling interval, snapshot can still contain a sample from the disabled window; when the flag is true again, PrometheusMetricVisitor exports it. Add an enable-generation (or a post-build enable check) so re-enabling cannot expose a snapshot from the disabled period.
Contributor
Author
There was a problem hiding this comment.
The mechanism is right, but the snapshot exported in either case is never older than the one a monitor that was never disabled would export at that moment.
- The monitor checks the flag every second. A disable that lasts until the next check drops the snapshot, and after re-enabling, the series are missing until a round completes. That is what dropping it is for: after a long disable, the first round can take a while, and a snapshot hours old must not be exported in the meantime (
testRoundsFollowTheIntervalAndStopWhileDisabled). - What the daemon does not see is a toggle shorter than one check, or a disable during a walk that is undone before the next check. The snapshot exported afterwards is then the latest round, taken before the disable or finishing as it happened, and
doris_fe_changelog_ivm_monitor_last_sample_timestamp_secondssays when it was taken. Nothing in it comes from a disabled period. - While the monitor is disabled, nothing is exported: the visitor checks
isEnabled(), and the next check drops the snapshot. - A check of the flag before publishing would not cover either case, since both checks see it on. An enable generation needs a callback on the config (
ConfHandler) for a window of under a second with no observable difference.
So I'm keeping it as it is.
Contributor
TPC-H: Total hot run time: 28347 ms |
Contributor
TPC-DS: Total hot run time: 151148 ms |
Contributor
ClickBench: Total hot run time: 24.54 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
6 of 13 tasks
…elog monitor ### What problem does this PR solve? Issue Number: apache#69907 Related PR: apache#69848 Problem Summary: The names of the monitor read as if it watched IVM only: `ChangelogIvmMonitor`, `enable_changelog_ivm_monitor`, `doris_fe_changelog_ivm_monitor_*`. It samples the changelog the tables keep, the table streams reading it, those users create as well as the internal streams of the IVM materialized views, and the IVM materialized views over those streams, so it is now named after the changelog: - `ChangelogIvmMonitor` -> `ChangelogMonitor`, `ChangelogIvmMonitorSnapshot` -> `ChangelogMonitorSnapshot`, `Env.getChangelogMonitor()`, `MetricVisitor.visitChangelogMonitor()`, and the daemon thread `changelog-monitor`. - FE configs `enable_changelog_ivm_monitor` -> `enable_changelog_monitor` and `changelog_ivm_monitor_interval_seconds` -> `changelog_monitor_interval_seconds`. - `doris_fe_changelog_ivm_monitor_last_sample_timestamp_seconds`, `doris_fe_changelog_ivm_monitor_sample_duration_seconds` and `doris_fe_changelog_ivm_monitor_sample_failures_total` -> `doris_fe_changelog_monitor_*`. - The tests `ChangelogMonitorTest`, `ChangelogMonitorMetricsTest` and `changelog_p0/test_changelog_monitor_metrics`. None of the old names has been released. Nothing else changes. ### Release note None ### Check List (For Author) - Test: Unit Test - `ChangelogMonitorTest`, `ChangelogMonitorMetricsTest` - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
…g monitor ### What problem does this PR solve? Issue Number: apache#69907 Related PR: apache#69848 Problem Summary: Where the monitor drops its snapshot, the comment said that once enabled again, the monitor must not export what it sampled before it was disabled. The monitor reads the flag at its checks, once a second, so this holds for a disable a check finds. A toggle between two checks, or a disable during a round undone before the next check, goes unseen, and the latest round is then exported as if the monitor had never been disabled. The comment now says what is dropped and when. No code changes. ### Release note None ### Check List (For Author) - Test: No need to test (a comment only) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
morningman
changed the title
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28739 ms |
Contributor
TPC-H: Total hot run time: 28233 ms |
Contributor
TPC-DS: Total hot run time: 151423 ms |
Contributor
ClickBench: Total hot run time: 24.3 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: #69907
Related PR: None
Problem Summary:
In short. Table streams and IVM materialized views export no metrics, and the signals an operator needs from them (is an MV fresh, is a stream about to fall behind its changelog TTL) are too costly to compute while
/metricsis scraped. This PR adds a daemon of the Master FE that samples into an immutable snapshot, renders the snapshot in/metrics, and puts the first series on it: the changelog kept by the tables, and the health of the sampling itself. The state of the streams and of the IVM materialized views comes with follow-up PRs on this framework.Background. A table with
changelog.enablekeeps a changelog, which table streams and IVM materialized views consume. Their state shows only in system tables (table_streams,table_stream_consumption,mv_infos()), which have to be queried one by one, andSHOW DATAlists the changelog size of each table asChangelogSize.The problem, and what it cost.
/metricssays how much changelog the tables keep, or whether the streams and IVM materialized views keep up with it.MetricRepo.getMetricrenders the whole/metricsresponse under one lock, so computing them there would make every scrape pay for a walk over the metadata.minormaxacross the FEs to 0.How this PR fixes it.
ChangelogMonitor, a daemon of the Master FE: everychangelog_monitor_interval_secondsit samples, builds an immutableChangelogMonitorSnapshot, and replaces the previous one at once. A round that fails keeps the previous snapshot and counts the failure.MetricVisitor.visitChangelogMonitor, besidevisitCloudTableStats, renders the series from the latest snapshot. Only the Master exports them: another FE exports none of them rather than 0, and a new Master exports none of the sampled series until its first round completes. The JSON and core outputs of/metricsrender nothing.doris_fe_changelog_retained_bytes{scope="single_replica"}doris_fe_changelog_retained_bytes{scope="all_replicas"}SHOW DATAlists asChangelogSize; not exported in cloud mode, where a tablet keeps one copy in the shared storagedoris_fe_changelog_monitor_last_sample_timestamp_secondsdoris_fe_changelog_monitor_sample_duration_secondsdoris_fe_changelog_monitor_sample_failures_totalThe last three let an alert tell stale data (
time() - doris_fe_changelog_monitor_last_sample_timestamp_seconds > 120, or the series absent) from a healthy state.enable_changelog_monitor(true), which takes effect withenable_feature_changelogonly, so a cluster without the changelog feature exports nothing new; andchangelog_monitor_interval_seconds(15), which the daemon checks every second, so a change applies at once.Results.
/metricsof the Master,enable_feature_changelogon/metricsof any other FEenable_feature_changelogorenable_changelog_monitoroffall_replicasagainst theChangelogSizetotal ofSHOW DATA PROPERTIES ("entire_warehouse" = "true")Classes, and how they call each other.
ChangelogMonitor(org.apache.doris.changelog): the daemon.buildSnapshotsumsOlapTable.getChangelogSize(true)andgetChangelogSize(false)over the tables of the internal catalog, from the statistics thatTabletStatMgrorCloudTabletStatMgrrefreshes.ChangelogMonitorSnapshot(org.apache.doris.changelog): what one round sampled; never changes once built.MetricVisitor.visitChangelogMonitor: implemented byPrometheusMetricVisitor, empty inJsonMetricVisitorandSimpleCoreMetricVisitor; called byMetricRepo.getMetricaftervisitCloudTableStats.Env: holds the monitor and starts it with the other daemons of the Master.Nothing else changes: no persisted metadata, no thrift or proto change, and the existing series are untouched.
Release note
With
enable_feature_changelogon, the Master FE exportsdoris_fe_changelog_retained_bytes{scope}and the health of its sampling,doris_fe_changelog_monitor_last_sample_timestamp_seconds,doris_fe_changelog_monitor_sample_duration_secondsanddoris_fe_changelog_monitor_sample_failures_total. New FE configsenable_changelog_monitorandchangelog_monitor_interval_seconds.Check List (For Author)
Test
Unit test:
ChangelogMonitorTestchecks that a round sums the changelog thatSHOW DATAreads from the table statistics, that a failed round keeps the previous snapshot and counts the failure, and that rounds follow the interval, a shorter interval applies at the next check, and a disabled monitor samples nothing and drops its snapshot.ChangelogMonitorMetricsTestchecks the exact text of the series, also throughMetricRepo.getMetric; only the failure counter before the first round; nothing on a Follower, or with the monitor or the changelog feature disabled;single_replicaonly in cloud mode; nothing in the JSON and core outputs.Regression test:
changelog_p0/test_changelog_monitor_metrics, run on a local shared-nothing cluster (not in cloud mode). On the/metricsof the Master,all_replicas(single_replicain cloud mode) equals theChangelogSizetotal ofSHOW DATA PROPERTIES ("entire_warehouse" = "true"); the health series are present and fresh; no other FE exports them; nothing once the monitor is disabled.Manual:
promtool check metricson the/metricsof the Master reports no parsing error and no lint finding on the new series.Behavior changed:
With
enable_feature_changelogon, the/metricsof the Master FE has the new series, and a daemon of the Master sums the table statistics every 15 seconds. Nothing changes otherwise.Does this need documentation?
Together with the rest of the changelog and IVM monitoring, in a follow-up doc PR.
Check List (For Reviewer who merge this PR)