feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

@timfish@lforst@anonrig@AbhiPrasad@github-advanced-security
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

@timfish@lforst@anonrig@AbhiPrasad@github-advanced-security
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

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

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

@timfish@lforst@anonrig@AbhiPrasad@github-advanced-security
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

@timfish@lforst@anonrig@AbhiPrasad@github-advanced-security
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

@timfish@lforst@anonrig@AbhiPrasad@github-advanced-security
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

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

feat(node): Rework ANR to use worker script via an integration - #9823

Merged
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script
Dec 19, 2023
Merged

feat(node): Rework ANR to use worker script via an integration#9823
AbhiPrasad merged 30 commits into
getsentry:developfrom
timfish:anr-worker-from-base64-script

Conversation

@timfish

@timfishtimfish commented Dec 13, 2023

Copy link
Copy Markdown
Collaborator

This PR reworks Node ANR detection to use a worker thread. Workers are usually started via a path to a source file but this can cause issues when bundlers are used. Instead, the worker code is bundled and included in the source as a base64 string which can be used to launch a worker via a data URL. The base64 code comes in at around 45KB.

Closes#9324 by adding trace context.

Also aids with solving getsentry/sentry-electron#784

Relies on #9018 being merged so module and app/device context are included.

Positives 👍

  • No extra processes
  • Only 10-15MB memory overhead (vs at least 50MB for a child process)
  • Uses inspector API so we can remove the websockets implementation
  • Doesn't require special cases/setup for Electron main process
  • No longer runs the app entry point again as the child process code
    • Closes some outstanding Electron ANR issues
    • ANR becomes just an integration since we don't need to intercept app execution in the child
    • Less confusing setup and less chance of running the app twice

Negatives 👎

  • Minimum supported Node version for ANR detection becomes v16 because Node 14 does not support data URLs for workers 😢

Usage

import*asSentryfrom'@sentry/node';Sentry.init({dsn: 'https://public@dsn.ingest.sentry.io/1337',integrations: [newSentry.Integrations.Anr({captureStackTrace: true,anrThreshold: 200})],});

@timfish
timfish marked this pull request as ready for review December 13, 2023 20:56
@timfish

This comment was marked as outdated.

Comment threadpackages/node-integration-tests/suites/anr/basic.js
Comment threadpackages/node-integration-tests/suites/anr/test.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/index.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts

const { poll } = watchdogTimer(createHrTimer, options.pollInterval, options.anrThreshold, watchdogTimeout);

parentPort?.on('message', (msg: { session: Session | undefined }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parentPort?.on('message',(msg: {session: Session|undefined})=>{
parentPort?.on('message',(msg: {session?: Session})=>{

Comment threadpackages/node/src/index.ts Outdated

const testScriptPath = path.resolve(__dirname, 'legacy.js');

childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (_, stdout) => {

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Comment threadrollup/bundleHelpers.js
Comment threadpackages/node/src/integrations/anr/worker.ts Outdated
Comment threadpackages/node/src/integrations/anr/worker.ts
Comment threadpackages/node/src/integrations/anr/index.ts
@timfish

Copy link
Copy Markdown
CollaboratorAuthor

This PR now also fetches the parentSpanId and I added some test asserts that check the app/device/os/culture context gets added to events.

@AbhiPrasad
AbhiPrasad merged commit ce9efc7 into getsentry:developDec 19, 2023
@timfish
timfish deleted the anr-worker-from-base64-script branch December 20, 2023 04:17
@lforst

Copy link
Copy Markdown
Contributor

Emitting files into our src folder is very bad for a lot of reasons - caching, build order, general reproducability. We need to change this.

@lforst

Copy link
Copy Markdown
Contributor

Pls do not do this immediately as I am restructuring our rollup configs right now and I am hotfixing this but this is high prio.

@lforst

Copy link
Copy Markdown
Contributor

Sorry I have to revert this PR. It has too many bad implications with the build process.

lforst pushed a commit that referenced this pull request Dec 20, 2023
@timfish
timfish restored the anr-worker-from-base64-script branch December 20, 2023 14:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node ANR tracking not getting trace context attached

5 participants

@timfish@lforst@anonrig@AbhiPrasad@github-advanced-security