-
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
[Telemetry] Track event loop delays on the server #101580
Conversation
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/event_loop_delays.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/event_loop_delays.ts
Outdated
Show resolved
Hide resolved
export function startTrackingEventLoopDelaysUsage( | ||
internalRepository: ISavedObjectsRepository, | ||
stopMonitoringEventLoop$: Subject<void>, | ||
collectionStartDelay = MONITOR_EVENT_LOOP_DELAYS_START, |
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.
thus, we start collecting data 1 min
after the start lifecycle of kibana_usage_collection
plugin.
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.
The monitoring of the event loop starts immediately. The first collection of the histogram happens after 1 minute. The daily histogram data is updated every 1 hour.
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/saved_objects.ts
Show resolved
Hide resolved
} | ||
|
||
export function serializeSavedObjectId({ date, pid }: { date: moment.MomentInput; pid: number }) { | ||
const formattedDate = moment(date).format('DDMMYYYY'); |
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.
The same comment about moment
usage when the standard library should do the trick:
const date = new Date();
console.log(`${date.getDate()}_${date.getMonth()}_${date.getFullYear()}`);
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.
I prefer using moment
for dates especially when it comes to formatting etc. .format('DDMMYYYY')
is a lot more readable and less error prone than the suggestion above IMO.
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.
is a lot more readable
I do agree it's more readable, I am just not sure that using moment
for such simple functionality justifies the performance penalty. Yeah, it's not significant per se, but it's a road to death by a thousand cuts.
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.
I'm generally against focusing on micro optimizations. There are a lot of bigger performance bottlenecks in an i/o project like kibana.
I dont think kibana would ever suffer in performance because JS can execute a moment
function 200k times per second while a native date would execute 800k times. I'd always favor readablity and maintanablity in these cases. Especially since we have moment
integrated in every place where we use dates even in eui components.
I did however replace moment.now()
with Date.now()
following your advice
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/rollups/daily.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/rollups/daily.test.ts
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/rollups/daily.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/event_loop_delays.ts
Outdated
Show resolved
Hide resolved
eventLoopDelaysCollector.reset(); | ||
} | ||
await storeHistogram(histogram, internalRepository); |
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.
NIT: technically, being inside a subscription, I think this await
does nothing. Not sure it would even protect against unresolved rejections?
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.
Technically it is exactly the same as without await
in this case since it is inside a .subscribe(..)
. I've just added it to explicitly denote that a Promise
is expected here, also if we want to add some logic in the future under this function it might be easy to miss adding await
/** | ||
* Roll daily indices every 24h | ||
*/ | ||
export const ROLL_DAILY_INDICES_INTERVAL = 24 * 60 * 60 * 1000; |
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.
optional nit: it's easy to forget updating the comment, so self-docuemented code FTW:
const Min = 60 * 1000;
const Hour = 60 * Min;
/** * Start rolling indices */
export const ROLL_DAILY_INDICES_INTERVAL = 24 * Hour
export const ROLL_INDICES_START = 5 * Min;
...llection/server/collectors/event_loop_delays/rollups/integration_tests/daily_rollups.test.ts
Outdated
Show resolved
Hide resolved
...llection/server/collectors/event_loop_delays/rollups/integration_tests/daily_rollups.test.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/rollups/daily.ts
Outdated
Show resolved
Hide resolved
...llection/server/collectors/event_loop_delays/rollups/integration_tests/daily_rollups.test.ts
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/saved_objects.test.ts
Outdated
Show resolved
Hide resolved
src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/saved_objects.ts
Outdated
Show resolved
Hide resolved
...llection/server/collectors/event_loop_delays/rollups/integration_tests/daily_rollups.test.ts
Outdated
Show resolved
Hide resolved
Was trying to think if there was any way this could break with the plan for clustering, but AFAICT we should be fine as long as the pid is always reported with the metrics. |
💚 Build SucceededMetrics [docs]Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Ahmad Bamieh <[email protected]>
Summary
Reports an array of daily histograms of delays in the event loop.
Uses
perf_hooks.monitorEventLoopDelay
to track the histogram with the defaultresolution
of10ms
.Event loop delays data is aggregated daily in a histogram per process ID.
The histogram is stored and updated in a saved object every 1 hour. This means that we'd maximum lose an hour worth of tracking on server shutdown.
Example payload
Schema
Notes
Once we have node
v15.9
we can utilize the nativeHistogram
provided by node to bake histograms into usage_counters to provide a histogram api in addition to thetotals
counter.Context: #101283