[ISSUE #190] Support queue-level consumer metrics behind a topic whitelist - #191
Open
xuanskyer wants to merge 2 commits into
Open
[ISSUE #190] Support queue-level consumer metrics behind a topic whitelist#191xuanskyer wants to merge 2 commits into
xuanskyer wants to merge 2 commits into
Conversation
Author
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 is the purpose of the change
Closes #190.
All consumer-side metrics are aggregated to broker granularity today, so a single lagging queue is invisible in Prometheus —
rocketmq_group_diffonly shows that the whole group is behind, and one has to fall back to the console to see per-queue offsets. This PR exposes the queue dimension that the collector already has in hand.MetricsCollectTask#collectConsumerOffsetiteratesconsumeStats.getOffsetTable(), which is aMap<MessageQueue, OffsetWrapper>, and already computeslagTimeone queue at a time. Both loops then fold the values into aHashMap<brokerName, Long>and discardqueueId. Publishing the per-queue values therefore needs no additional admin call —queryMsgByOffsetis still invoked exactly once per queue, as before.Note that the label list for this already exists in
RMQMetricsCollector(GROUP_PULL_LATENCY_LABEL_NAMES, containingqueueid) but has never been referenced by any metric.Brief changelog
New metrics, all labelled
cluster, broker, topic, group, queueid:rocketmq_queue_group_diffbrokerOffset - consumerOffset)rocketmq_queue_group_get_latency_by_storetimelagTime)rocketmq_queue_consumer_offsetQueue-level series multiply the consumer series count by the number of queues per broker, so they are disabled by default and gated by an explicit topic whitelist:
Changes:
RMQConfigure— newqueueLevelTopicsproperty, parsed once in the setter into aSet, plusisQueueLevelTopic(String).ConsumerQueueMetric(new) — metric key, i.e.ConsumerMetricplusqueueId.RMQMetricsCollector— three caches reusing the existingoutOfTimeSecondsexpiry,collectQueueGroupNums()and threeaddXxxMetric()methods, following the existinggroupDiffpattern.MetricsCollectTask— publishes the per-queue values inside the two existing loops, guarded by the whitelist. The broker-level aggregation is left exactly as is, so existing dashboards and alerts are unaffected.application.yml— documents the new option.The change is purely additive: no existing line of behaviour was modified.
The second commit fixes a pre-existing checkstyle violation (
if(missing a space,MetricsCollectTask.java) that currently makesmvn clean installfail on master regardless of this PR — without it the build command below cannot pass. Happy to split it out if you prefer.Verifying this change
mvn -B clean install -DskipITs— BUILD SUCCESS,Tests run: 5, Failures: 0, Errors: 0mvn -B clean apache-rat:check checkstyle:checkstyle— RAT summaryUnapproved: 0, unknown: 0, approved: 39; checkstyle clean after the second commitRMQConfigureQueueLevelTestcovers the whitelist parsing: disabled by default, blank stays disabled, comma list is trimmed and matched exactly (topic-amust not enabletopic-a-extra, otherwise the series budget silently blows up),*wildcard, and resetting back to disabled. The test was also verified in reverse — replacing the exact match withstartsWithmakes it fail as expected.Note that
findbugs:findbugsfrom the checklist was not run: the plugin does not support the JDK used locally (17).spotbugswould be the modern replacement, but that seemed out of scope for this PR.One thing worth mentioning for the maintainers: the existing test sources are JUnit 4, while surefire 3.2.2 auto-selects the JUnit Platform provider, so
mvn testcurrently reportsTests run: 0and no existing test actually executes. The new test is therefore written against JUnit 5 so that it really runs. Addingjunit-vintage-enginewould revive the existing ones, but again felt out of scope here.