Skip to content

rabbit_osiris_metrics: Use committed_offset counter #15390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
the-mikedavis wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

rabbit_osiris_metrics: Use committed_offset counter #15390

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions deps/rabbit/src/rabbit_osiris_metrics.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ handle_cast(_Request, State) ->
handle_info(tick, #state{timeout = Timeout} = State) ->
Data = osiris_counters:overview(),
_ = maps:map(
fun ({osiris_writer, QName}, #{offset := Offs,
fun ({osiris_writer, QName}, #{committed_offset := COffs,
Copy link
Collaborator Author

@the-mikedavis the-mikedavis Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check the feature flag and use it to decide whether we should use offset or committed_offset?

Copy link
Collaborator

@michaelklishin michaelklishin Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If those values can come from other nodes, we do. If they are entirely local state, maybe not.

Copy link
Contributor

@acogoluegnes acogoluegnes Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not crash as committed_offset is always available, but the stats will be slightly inaccurate before the changes in #15225 are enforced with the 4.3 feature flag. committed_offset is the offset of the last committed chunk pre-15225, so it is behind by the number of messages in the chunk compared to the actual last offset.

This should fix itself when the upgrade is done and after the streams restart, so I guess it is reasonable considering it is just stats.

WDYT @kjnilsson ?

Copy link
Contributor

@kjnilsson kjnilsson Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it is mostly informational at this point so shouldn't matter, let's not bother the ff subsystem if we don't have to.

first_offset := FstOffs}) ->
COffs = Offs + 1 - FstOffs,
rabbit_core_metrics:queue_stats(QName, COffs, 0, COffs, 0),
Offsets = COffs + 1 - FstOffs,
rabbit_core_metrics:queue_stats(QName, Offsets, 0, Offsets, 0),
Infos = try
%% TODO complete stats!
case rabbit_amqqueue:lookup(QName) of
Expand Down
Loading