-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[NIFI-15162] Flow metrics reporting strategy for nifi metrics api #10501
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
Conversation
5c41050 to
a4a5004
Compare
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this improvement @esecules.
The change looks straightforward, I noted a few implementation recommendations.
...nifi-web-api/src/main/java/org/apache/nifi/web/api/request/FlowMetricsReportingStrategy.java
Outdated
Show resolved
Hide resolved
...nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java
Outdated
Show resolved
Hide resolved
...ifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiServiceFacade.java
Outdated
Show resolved
Hide resolved
...ifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiServiceFacade.java
Show resolved
Hide resolved
...ework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java
Outdated
Show resolved
Hide resolved
...nifi-web-api/src/main/java/org/apache/nifi/web/api/request/FlowMetricsReportingStrategy.java
Outdated
Show resolved
Hide resolved
…er to filter what metrics the flow metrics registry emits.
Co-authored-by: David Handermann <[email protected]>
…en generating the flow metrics, fixing docstrings.
|
@exceptionfactory would you like me to squash the 3 commits here into 1 or are we good? |
Thanks for asking, no need to squash commits during the review, I can do it when ready on merge. |
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updates @esecules! This looks close to completion, but I noted where the updated method argument appears to require updating references in the method to change the value comparison, then this should be ready to go forward.
...ifi-web/nifi-web-api/src/main/java/org/apache/nifi/prometheusutil/PrometheusMetricsUtil.java
Show resolved
Hide resolved
…nitions for the metrics reporting strategy
…cause we do not make use of any of the extra descriptions anymore.
|
Good catch on the equality check @exceptionfactory! I've made the changes and also noticed i could simplify the enum definition for the reporting strategy since we don't make use of the descriptions anywhere since its not a parameter in the UI anymore like it was in the reporting task. |
|
I checked the two failing system tests and they both pass on my macbook using java 21 and 25 according to each test failure. |
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the simplified enum approach @esecules, that looks good. I noted one more minor adjustment for equals checking of the enum values, then this should be ready to go.
...ifi-web/nifi-web-api/src/main/java/org/apache/nifi/prometheusutil/PrometheusMetricsUtil.java
Outdated
Show resolved
Hide resolved
...ifi-web/nifi-web-api/src/main/java/org/apache/nifi/prometheusutil/PrometheusMetricsUtil.java
Outdated
Show resolved
Hide resolved
Co-authored-by: David Handermann <[email protected]>
exceptionfactory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @esecules! +1 merging
Summary
NIFI-15162
This reintroduces the metrics reporting strategy feature from the PrometheusReportingTask. It can be used to only collect flow metrics related to process groups. This will help reduce the CPU load of collection metrics from larger flows when the flow admin wants to omit individual processor and connection metrics already.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation