-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Adds support for event loop utilization to the core metrics service #153717
Conversation
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.
self review
defaultMessage: 'Heap used out of {heapTotal}', | ||
values: { heapTotal: numeral(metrics.process.memory.heap.size_limit).format('0.00 b') }, |
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.
To avoid restructuring the whole status page, I combined the two separate heap tiles into one that includes both heapUsedInBytes
and sizeLimit
.
}, | ||
}), | ||
value: metrics.process.event_loop_utilization.utilization, | ||
type: 'float', |
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.
We're autoformatting floats to two decimal places... it's nice to see the utilization values with a bit more precision, but I don't know if it's worth a one-off implementation just for this metric. I tried updating all floats to five decimal places, but it blew up the layout, so I decided to leave it alone since this is a nice-to-have item anyway 🤷
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.
Yeah, it's fine ihmo. That page has been a pain every time we want to change/add/remove anything...
@@ -55,6 +55,11 @@ export function getEcsOpsMetricsLog(metrics: OpsMetrics) { | |||
).format('0.000')} }` | |||
: ''; | |||
|
|||
const eventLoopUtilizationVal = process?.event_loop_utilization; | |||
const eventLoopUtilizationMsg = eventLoopUtilizationVal | |||
? ` utilization: ${numeral(process?.event_loop_utilization.utilization).format('0.00000')}` |
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.
Including active/idle here didn't seem useful enough to justify the increased message length. But it is easy to add if anyone feels otherwise.
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.
Not sure either to see how they could be useful. The utilization ratio seems the only valuable info
efe9de7
to
934f88b
Compare
Pinging @elastic/kibana-core (Team:Core) |
@elasticmachine merge upstream |
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.
Looking good to me (once the tests are fixed!)
}, | ||
}), | ||
value: metrics.process.event_loop_utilization.utilization, | ||
type: 'float', |
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.
Yeah, it's fine ihmo. That page has been a pain every time we want to change/add/remove anything...
packages/core/metrics/core-metrics-collectors-server-internal/src/process.test.ts
Outdated
Show resolved
Hide resolved
@@ -55,6 +55,11 @@ export function getEcsOpsMetricsLog(metrics: OpsMetrics) { | |||
).format('0.000')} }` | |||
: ''; | |||
|
|||
const eventLoopUtilizationVal = process?.event_loop_utilization; | |||
const eventLoopUtilizationMsg = eventLoopUtilizationVal | |||
? ` utilization: ${numeral(process?.event_loop_utilization.utilization).format('0.00000')}` |
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.
Not sure either to see how they could be useful. The utilization ratio seems the only valuable info
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.
LGTM! It is good that you skipped using the ELU class I have from the POC because it is very specific to the collector (to avoid double logging and double incrementing the counters). Using the nodeJS eventLoopUtilization
method directly as you did looks correct to me 👍
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @lukeelmers |
This updates the metrics service with a collector for event loop utilization, based on some prior art by @Bamieh ❤️
To learn more about ELU, I recommend this article from nodesource.
Things included:
/api/status
,/api/stats
are automatically updated as a result of this changeThings not included:
Note to reviewers: I restructured one of the test files a bit, so reviewing will be easier if you view the diff with
Hide whitespace
enabled.