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.

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

[feat][broker] PIP-264: Add OpenTelemetry broker replicator metrics #22972

Merged
merged 54 commits into from
Jun 25, 2024

Conversation

dragosvictor
Copy link
Contributor

@dragosvictor dragosvictor commented Jun 25, 2024

PIP-264

Motivation

Adds existing broker replicator metrics (documented by https://pulsar.apache.org/docs/next/reference-metrics/#replication-metrics-1) to the OpenTelemetry pipeline.

Modifications

  • Adds the replication metrics, as described by [feat][doc] PIP-264: Add replication metrics reference pulsar-site#926, to the collector runs.
  • Renamed Replicator#getStats to Replicator#computeStats, as it modifies the underlying object in the process. Added a simple getter Replicator#getStats for the object itself. This helps clarify the intent of the methods.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added test ReplicatorTest#testReplicationMetrics
  • Updated test ReplicatorTest#testResumptionAfterBacklogRelaxed

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

Documentation

Matching PR in forked repository

PR in forked repository: dragosvictor#26

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jun 25, 2024
@dragosvictor dragosvictor marked this pull request as ready for review June 25, 2024 04:49
@merlimat merlimat merged commit f323342 into apache:master Jun 25, 2024
53 of 54 checks passed
@dragosvictor dragosvictor deleted the pip-264-replication-metrics branch June 25, 2024 17:32
@lhotari
Copy link
Member

lhotari commented Sep 5, 2024

@dragosvictor is this change intended also for 3.3.x ? Do you see any reasons why we shouldn't cherry-pick this to branch-3.3 ? The reason why I'm asking is that #23236 would apply cleanly if we first cherry-pick this PR. /cc @BewareMyPower @RobertIndie

@dragosvictor
Copy link
Contributor Author

It's not intended for 3.3, but it won't hurt either. No problem cherry-picking it.

@lhotari
Copy link
Member

lhotari commented Sep 5, 2024

Checked again and perhaps minimizing the risk is worthwhile if it's not needed in 3.3.x. It's not a big deal to backport #23236 to branch-3.3.

@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants