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

[Issue 9804] Allow to enable or disable the cursor metrics #9814

Merged
merged 4 commits into from
Mar 8, 2021

Conversation

linlinnn
Copy link
Contributor

@linlinnn linlinnn commented Mar 5, 2021

Fixes #9804

Motivation

Allow to enable or disable the cursor metrics

Modifications

add config item exposeManagedCursorMetricsInPrometheus to allow user to enable or disable the cursor metrics

Verifying this change

  • Make sure that the change passes the CI checks.

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Could you please add some tests?

@codelipenghui
Copy link
Contributor

@limingnihao Please help review this PR, thanks.

// generate managedCursor metrics
parseMetricsToPrometheusMetrics(new ManagedCursorMetrics(pulsar).generate(),
clusterName, Collector.Type.GAUGE, stream);
if(includeManagedCursorMetrics){
Copy link
Contributor

@murong00 murong00 Mar 5, 2021

Choose a reason for hiding this comment

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

Why not use pulsar.getConfiguration().isExposeManagedCursorMetricsInPrometheus() here? BTW, please note the code style if (...) {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for being consistent with includeConsumerMetrics, includeProducerMetrics etc.

Copy link
Contributor

@MarvinCai MarvinCai left a comment

Choose a reason for hiding this comment

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

LGTM, please add a test case or update existing test case in PrometheusMetricsTest to verify the behavior.

@linlinnn
Copy link
Contributor Author

linlinnn commented Mar 6, 2021

/pulsarbot run-failure-checks

@linlinnn linlinnn requested a review from murong00 March 6, 2021 10:47
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

Please add a testcase

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Looks great

Thanks

@codelipenghui
Copy link
Contributor

@murong00 Could you please review this PR again?

@linlinnn
Copy link
Contributor Author

linlinnn commented Mar 8, 2021

/pulsarbot run-failure-checks

@sijie sijie merged commit a23fb03 into apache:master Mar 8, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Mar 23, 2021
codelipenghui pushed a commit that referenced this pull request Mar 23, 2021
Fixes #9804

Allow to enable or disable the cursor metrics

add config item `exposeManagedCursorMetricsInPrometheus`  to allow user to enable or disable the cursor metrics

(cherry picked from commit a23fb03)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to enable or disable the cursor metrics in the broker.conf
7 participants