Skip to content
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

Jump to bottom

[improve] [pip] PIP-355: Enhancing Broker-Level Metrics for Pulsar #22778

Merged
merged 6 commits into from
Jun 11, 2024
Merged

[improve] [pip] PIP-355: Enhancing Broker-Level Metrics for Pulsar #22778

merged 6 commits into from
Jun 11, 2024

Conversation

hangc0276
Copy link
Contributor

Fixes #xyz

Main Issue: #xyz

PIP: #xyz

Motivation

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added PIP doc-required Your PR changes impact docs and you will update later. labels May 25, 2024
@hangc0276 hangc0276 self-assigned this May 25, 2024
@hangc0276 hangc0276 added this to the 3.4.0 milestone May 25, 2024
@hangc0276 hangc0276 mentioned this pull request May 25, 2024
Merged
15 tasks
horizonzy
horizonzy reviewed May 25, 2024
View reviewed changes
Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

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

Overall LGTM. But I have a question, do we need to consider the system topic, the system topic producer and consumer may be not used by users directly, but it looks like the metrics will include the traffic

@asafm
Copy link
Contributor

asafm commented May 25, 2024

@dragosvictor Needs to be aware of this, as he is working on converting the existing metrics to OTel.

asafm
asafm reviewed May 25, 2024
View reviewed changes
pip/pip-355.md Outdated Show resolved Hide resolved
@hangc0276
Copy link
Contributor Author

Overall LGTM. But I have a question, do we need to consider the system topic, the system topic producer and consumer may be not used by users directly, but it looks like the metrics will include the traffic

@horizonzy good point. I update the document and include the system topics for traffic in

@hangc0276
Copy link
Contributor Author

@asafm @dragosvictor @codelipenghui @horizonzy I updated the document, and please help take a look, thanks.

dragosvictor
dragosvictor reviewed May 31, 2024
View reviewed changes
pip/pip-355.md Outdated Show resolved Hide resolved
dragosvictor
dragosvictor approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@dragosvictor dragosvictor left a comment

Choose a reason for hiding this comment

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

LGTM!

@asafm
Copy link
Contributor

asafm commented Jun 1, 2024

@hangc0276 I added one more comment reply which is important IMO.

@hangc0276 @dragosvictor
Update pip/pip-355.md
2ac02c0 
Co-authored-by: Dragos Misca <dragosvictor@users.noreply.github.com>
@hangc0276 hangc0276 merged commit 6b76544 into apache:master Jun 11, 2024
20 checks passed
hangc0276 added a commit that referenced this pull request Jun 12, 2024
@hangc0276
[improve] [broker] PIP-355: Enhancing Broker-Level Metrics for Pulsar (…
c724f02 
…#22779)

PIP: #22778
hangc0276 added a commit that referenced this pull request Jun 12, 2024
@hangc0276
[improve] [broker] PIP-355: Enhancing Broker-Level Metrics for Pulsar (…
49b0378 
…#22779)

PIP: #22778
(cherry picked from commit c724f02)
@hangc0276 hangc0276 mentioned this pull request Jul 17, 2024
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asafm asafm asafm left review comments

@dragosvictor dragosvictor dragosvictor approved these changes

@horizonzy horizonzy horizonzy left review comments

@zymap zymap zymap approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@eolivelli eolivelli Awaiting requested review from eolivelli

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

Assignees

@hangc0276 hangc0276

Labels
doc-required Your PR changes impact docs and you will update later. PIP
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@hangc0276 @asafm @dragosvictor @codelipenghui @horizonzy @zymap