-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
NIFI-14077 Add ProcessGroup Performance Metrics to Prometheus #9577
base: main
Are you sure you want to change the base?
Conversation
…TAL_TASK_DURATION to ProcessGroups, add allable values to the documentation of the flow metrics API.
I suspect the macos system test is flakey so I am rerunning it on my fork https://github.com/esecules/nifi/actions/runs/12285436758?pr=3 EDIT: |
You're welcome, I have recently noticed a couple flaky tests in the system-tests workflow. |
Looks like the retry failed still 😔, however it's passing in my fork and my machine (MacBook). Do these tests run in parallel and might interfere with each other? Or the tests might be running on an underpowered server and the 60 second timeout isn't sufficient for GitHub Actions? Github's macos runners do only have 3 cores. Ubuntu runners have 4 cores. I am assuming apache/nifi uses the default runners. |
Looks like system tests passed on retry! Thanks for all the retries. |
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 proposing these changes @esecules. I'm not complete sure about introducing the additional gauges because the processing performance information can be disabled. However, it looks like the null handling accounts for that fact, so this approach may be sufficient. I noted a few minor recommendations.
...nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/prometheusutil/NiFiMetricsRegistry.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/prometheusutil/NiFiMetricsRegistry.java
Outdated
Show resolved
Hide resolved
.../nifi-web/nifi-web-api/src/main/java/org/apache/nifi/prometheusutil/NiFiMetricsRegistry.java
Outdated
Show resolved
Hide resolved
I'll check if I already added test coverage for when the performance feature is disabled and I'll add it if it's missing. |
I've looked over the testing for this module. I don't see any interactions with settings so I'll add a test case to make sure we continue to handle null performance information. |
…rmance stats are properly handled when they're null or present
Added the tests which validate that we don't blow up when the performance status is null. |
Co-authored-by: David Handermann <[email protected]>
Summary
NIFI-14077
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
(no new deps)
LICENSE
andNOTICE
filesDocumentation