-
Notifications
You must be signed in to change notification settings - Fork 274
chore: Make periodic measurements customizable #3291
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
base: main
Are you sure you want to change the base?
Conversation
… default This is a costly probe that we've seen take 1.3 seconds to complete. At 5-second periodicity, it runs 26% of the time. There is an additional cost of ensuring the return value of Process.list() is a consistent snapshot of the system processes, we do not really need it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3291 +/- ##
==========================================
- Coverage 71.12% 71.05% -0.08%
==========================================
Files 176 177 +1
Lines 9366 9449 +83
Branches 318 336 +18
==========================================
+ Hits 6662 6714 +52
- Misses 2702 2733 +31
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c74aff7 to
8ec32d8
Compare
…ement by default" This reverts commit 76e8579.
|
Would we be able to still maintain a per-process probe without manual intervention? It is useful to have some stats on this over time and over various releases - if it's costly perhaps it could/should run in a much sparser frequency? Or if there's a way to not pay the cost for the consistent snapshot perhaps there's a cheaper alternative? |
|
@msfstef sorry for making it unnecessarily confusing. I have reverted the change that disabled memory collection as it didn't have a deciding impact on performance. We can make it even less noticeable by using process iteration in the same way as I've opened an issue for that. |
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.
@alco thanks for clarifying as I couldn't match the PR desc to the code haha - this looks good!
Disable ApplicationTelemetry.process_memory() periodic measurement by default.
This is a costly probe that we've seen take 1.3 seconds to complete. At 5-second periodicity, it runs 26% of the time. There is an additional cost of ensuring the return value of Process.list() is a consistent snapshot of the system processes, we do not really need it.
Top processes by reductions or memory can be observed under human supervision by running
etop(), as inThis PR also introduces the possibility of passing custom periodic measurements to be run at the same cadence as the builtin ones. This is done to make it easier to add probes in production without havgint to update the Electric dependency version every time.