Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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" + '
feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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('^' + ".*" + ' feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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('^' + ".*" + ' feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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" + ' feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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('^' + ".*" + ' feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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('^' + ".*" + ' feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}
, '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); } })(); })(); feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript · GitHub
Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions packages/node/src/integrations/tracing/fastify.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,14 +61,27 @@ const _fastifyIntegration = (() => {
*/
export const fastifyIntegration = defineIntegration(_fastifyIntegration);

interface FastifyHandlerOptions {
/**
* Callback method deciding whether error should be captured and sent to Sentry
* @param error Captured middleware error
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;

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.

It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

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.

for that we need to depend on the fastify package, which we cannot do in the generic node package.

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.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

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.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8

It was a conscious decision not to do this.

Perhaps, a dummy interface can be used. After all, only a little piece of it is used.

Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

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.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.

Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

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.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.

I encourage you to pull the repo and try it out.

}

/**
* Setup an error handler for Fastify.
*/
export function setupFastifyErrorHandler(fastify: Fastify): void {
export function setupFastifyErrorHandler(fastify: Fastify, options?: FastifyHandlerOptions): void {
const plugin = Object.assign(
function (fastify: Fastify, _options: unknown, done: () => void): void {
fastify.addHook('onError', async (_request, _reply, error) => {
captureException(error);
const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;

fastify.addHook('onError', async (request, reply, error) => {
Comment on lines +79 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

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.

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.

Reply and Error are not the same since the reply could have been sent before the error occurred.

if (shouldHandleError(request, reply, error)) {
captureException(error, { mechanism: { type: 'middleware', handled: false } });
}
});

// registering `onRequest` hook here instead of using Otel `onRequest` callback b/c `onRequest` hook
Expand DownExpand Up@@ -131,3 +144,10 @@ function addFastifySpanAttributes(span: Span): void {
span.updateName(name.replace(/^fastify -> /, ''));
}
}

/** Returns true if response code is internal server error */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return reply.statusCode >= 500;

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.

statusCode can be 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.

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

statusCode can be undefined

Could probably implement this similar to what's been done in the express integration.

}