Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Update profiletrigger: switch from vsz to rss and add threshold for heap as well #1914

Merged
merged 3 commits into from
Oct 14, 2020

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Oct 2, 2020

  • old version used to look at Memstats.Sys, which was the entire virtual
    memory space, and effectively useless. Hence also why it's been hidden
    in the dashboard since forever. Most people expect this would
    look effectively at rss, so that's what we do now. + optional
    parameter to look at (only) the heap specifically
  • change default collection interval to 10s. That's what we've been
    running in prod for years without any issues.

* old version used to look at Memstats.Sys, which was the entire virtual
  memory space, and effectively useless. Hence also why it's been hidden
  in the dashboard since forever.   Most people expect this would
  look effectively at rss, so that's what we do now. + optional
  parameter to look at (only) the heap specifically
* change default collection interval to 10s. That's what we've been
  running in prod for years without any issues.
Copy link
Contributor

@robert-milan robert-milan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the change for clarification already noted, other than that looks fine, but I haven't reviewed the library code from profiletrigger so I am unaware of all those changes. Would you like me to take a look at those?

@Dieterbe
Copy link
Contributor Author

Would you like me to take a look at those?

nah those went through review already in that repo.

@Dieterbe Dieterbe merged commit 63bf743 into master Oct 14, 2020
@Dieterbe Dieterbe deleted the update-profiletrigger branch October 14, 2020 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants