Skip to content

[feat][broker] add consumer first consume/flow timestamp #24247

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattisonchao
Copy link
Member

Motivation

This PR supports two stats for the consumer side to help identify the first message dispatch latency.

Modifications

  • Add stats firstConsumedFlowTimestamp
  • Add stats firstConsumedTimestamp

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@mattisonchao mattisonchao self-assigned this May 5, 2025
@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label May 5, 2025
@mattisonchao mattisonchao changed the title feat(stats): add consumer first consume/flow timestamp [feat][broker] add consumer first consume/flow timestamp May 5, 2025
Comment on lines +440 to +442
if (firstConsumedTimestamp == 0) {
firstConsumedTimestamp = System.currentTimeMillis();
}
Copy link
Member

Choose a reason for hiding this comment

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

just wondering if this makes sense since flowPermits method would get called before messages get sent to a consumer?

Copy link
Member

Choose a reason for hiding this comment

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

oh sorry, there are 2 separate fields. Just wondering where lastConsumedTimestamp is set. wouldn't it be set in a similar way as firstConsumedTimestamp?

Copy link
Member Author

@mattisonchao mattisonchao May 5, 2025

Choose a reason for hiding this comment

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

oh sorry, there are 2 separate fields. Just wondering where lastConsumedTimestamp is set. wouldn't it be set in a similar way as firstConsumedTimestamp?

IMO, we should also move lastConsumedTimestamp to success sent. That will be more accurate, but I don't want to make this PR more complex. :/

Copy link
Contributor

@liangyepianzhou liangyepianzhou left a comment

Choose a reason for hiding this comment

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

LGTM

@mattisonchao mattisonchao requested a review from lhotari May 20, 2025 09:04
@mattisonchao mattisonchao reopened this May 20, 2025
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.75%. Comparing base (bbc6224) to head (55c8ee0).
Report is 1108 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #24247       +/-   ##
=============================================
- Coverage     73.57%   39.75%   -33.83%     
+ Complexity    32624    13406    -19218     
=============================================
  Files          1877     1809       -68     
  Lines        139502   145485     +5983     
  Branches      15299    17076     +1777     
=============================================
- Hits         102638    57831    -44807     
- Misses        28908    80156    +51248     
+ Partials       7956     7498      -458     
Flag Coverage Δ
inttests 27.73% <85.71%> (+3.14%) ⬆️
systests 24.06% <85.71%> (-0.26%) ⬇️
unittests 35.59% <100.00%> (-37.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ava/org/apache/pulsar/broker/service/Consumer.java 63.91% <100.00%> (-22.76%) ⬇️
.../common/policies/data/stats/ConsumerStatsImpl.java 100.00% <ø> (+2.38%) ⬆️

... and 1673 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-required Your PR changes impact docs and you will update later. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants