Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805

Closed
andresilva wants to merge 3 commits into
masterfrom
andre/sysinfo-process-leak
Closed

service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom
andre/sysinfo-process-leak

Conversation

@andresilva

@andresilvaandresilva commented Aug 3, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.

Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.

Screenshot_2020-08-03_18-33-25

@andresilvaandresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 3, 2020
@andresilva
andresilva requested a review from bkchrAugust 3, 2020 17:39
@andresilvaandresilva mentioned this pull request Aug 3, 2020
Closed
@andresilva

andresilva commented Aug 3, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.

edit: I'll do that.

@andresilvaandresilva added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 3, 2020
@andresilvaandresilva changed the title service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 4, 2020
@andresilva
andresilvaforce-pushed the andre/sysinfo-process-leak branch from 4cc3bb0 to 18e66e5CompareAugust 4, 2020 15:27
@andresilva

andresilva commented Aug 4, 2020

Copy link
Copy Markdown
ContributorAuthor

Screenshot_2020-08-04_16-36-28

Results on Linux without sysinfo.

[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva

andresilva commented Aug 4, 2020

Copy link
Copy Markdown
ContributorAuthor

Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.

@andresilvaandresilva mentioned this pull request Aug 5, 2020
Merged
@mxindenmxinden mentioned this pull request Aug 24, 2020
Merged
@tomakatomaka mentioned this pull request Sep 7, 2020
Merged
@tomakatomaka mentioned this pull request Sep 22, 2020
Merged
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees

No one assigned

Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
service: don't use sysinfo on Linux for metric collection by andresilva · Pull Request #6805 · paritytech/substrate · GitHub
Skip to content

This repository was archived by the owner on Nov 15, 2023. It is now read-only.

service: don't use sysinfo on Linux for metric collection - #6805
Closed
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Closed
service: don't use sysinfo on Linux for metric collection#6805
andresilva wants to merge 3 commits into
masterfrom 
andre/sysinfo-process-leak
Conversation
@andresilva
@andresilvaandresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
This PR updates our metrics code to not use the sysinfo crate on Linux and instead get all of the data through procfs.
Additionally it fixes an issue on platform that still use sysinfo: We were tracking metrics of all processes running in the node through sysinfo which in turn will cache file descriptors of all accesses to /proc/<pid>/stat. After this PR we will only collect metrics for the process of the node itself. Unfortunately sysinfo will still keep track of all threads spawned by the node and will keep a file descriptor for each /proc/<pid>/task/<tid>/stat and there is no way to avoid this currently. These will be bound by the number of threads we spawn though and they should not leak (i.e. they'll be collected after a given threads exits). Below you can see a plot on the number of file handles for a Kusama node before/after this change.
Screenshot_2020-08-03_18-33-25
@andresilvaandresilva
added

A0-please_review
Pull request needs code review.
B0-silent
Changes should not be mentioned in any release notes
C1-low
PR touches the given topic and has a low impact on builders.
labels
Aug 3, 2020
@andresilva
andresilva
requested a review
from bkchrAugust 3, 2020 17:39
@andresilvaandresilva
mentioned this pull request
Aug 3, 2020

Closed

@andresilva
andresilva
commented
Aug 3, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I might be able to drop the dependency on sysinfo in this PR for Linux and just do everything through procfs instead.
edit: I'll do that.
@andresilvaandresilva
added
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 3, 2020
@andresilvaandresilva
changed the title
service: don't track metrics of all processes in systemservice: don't use sysinfo on Linux for metric collectionAug 4, 2020
@andresilvaandresilva
removed
the 
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Aug 4, 2020
@andresilva
andresilvaforce-pushed
the
andre/sysinfo-process-leak
branch
from
4cc3bb0 to
18e66e5CompareAugust 4, 2020 15:27
@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Screenshot_2020-08-04_16-36-28
Results on Linux without sysinfo.
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | wc -l
236
[nix-shell:~/Projects/parity/polkadot]$ lsof -p $(pidof polkadot) | grep "/proc" | wc -l
0

@andresilva
andresilva
commented
Aug 4, 2020

Copy link


Copy Markdown

ContributorAuthor
Closing this PR and will instead remove all system metric reporting. These metrics can easily be collected by an external process https://github.com/prometheus/node_exporter.
@andresilvaandresilva
mentioned this pull request
Aug 5, 2020
 Merged

@mxindenmxinden
mentioned this pull request
Aug 24, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 7, 2020
 Merged

@tomakatomaka
mentioned this pull request
Sep 22, 2020
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@bkchrbkchrAwaiting requested review from bkchr

Assignees


No one assigned


Labels

A0-please_reviewPull request needs code review.B0-silentChanges should not be mentioned in any release notesC1-lowPR touches the given topic and has a low impact on builders.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

1 participant

@andresilva