-
Notifications
You must be signed in to change notification settings - Fork 128
docs: Add Exclude HTTP Endpoints in Node.js #2073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jugal Kishore <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude /review |
|
Claude finished @crazyuploader's task —— View job PR Review CompleteI've reviewed the documentation changes for excluding HTTP endpoints in Node.js. The content is technically accurate and well-structured overall, with a few minor improvements suggested. Review SummaryWhat was reviewed:
Findings: I've provided 5 inline comments with specific suggestions:
Strengths:
Sources consulted:
The documentation provides valuable guidance for developers who need to exclude health checks and other non-business endpoints from tracing, which is a common requirement in production environments. |
| tags: [SigNoz Cloud, Self-Host] | ||
| --- | ||
|
|
||
| import GetHelp from '@/components/shared/get-help.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing import statement for Tabs component. Other documentation files in this directory include:
import Tabs from '@theme/Tabs'
import TabItem from '@theme/TabItem'Add these imports after the GetHelp import.
| The `ignoreIncomingRequestHook(req)` callback runs before spans are created for each request. | ||
|
|
||
| If this function returns `true`, that specific request will be ignored and no span will be generated for that endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation could be more precise. According to the official OpenTelemetry types, ignoreIncomingRequestHook returns a boolean where true means "exclude this request from tracing."
| The `ignoreIncomingRequestHook(req)` callback runs before spans are created for each request. | |
| If this function returns `true`, that specific request will be ignored and no span will be generated for that endpoint. | |
| The `ignoreIncomingRequestHook(req)` function evaluates each incoming HTTP request before span creation. | |
| When this function returns `true`, the request is excluded from tracing and no span will be created for that endpoint. |
|
|
||
| ## Exclude HTTP Endpoints | ||
|
|
||
| The `ignoreIncomingRequestHook(req)` function gives access to each incoming request. We can match `req.url`, headers, or methods (GET, POST, etc.) to decide which request should be ignored from traces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor clarification: The function receives an IncomingMessage object, not just any request. Consider being more specific:
| The `ignoreIncomingRequestHook(req)` function gives access to each incoming request. We can match `req.url`, headers, or methods (GET, POST, etc.) to decide which request should be ignored from traces. | |
| The `ignoreIncomingRequestHook(req)` function gives access to each incoming HTTP request's `IncomingMessage` object. You can inspect `req.url`, headers, or HTTP methods (GET, POST, etc.) to decide which requests should be excluded from traces. |
| // Ignore any requests starting with /internal or /health | ||
| const httpInstrumentation = new HttpInstrumentation({ | ||
| ignoreIncomingRequestHook(req) { | ||
| return req.url.startsWith('/health') || req.url.startsWith('/internal') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Using req.url.startsWith() without null checking could cause issues if req.url is undefined. Consider using optional chaining:
| return req.url.startsWith('/health') || req.url.startsWith('/internal') | |
| const httpInstrumentation = new HttpInstrumentation({ | |
| ignoreIncomingRequestHook(req) { | |
| return req.url?.startsWith('/health') || req.url?.startsWith('/internal') | |
| }, | |
| }) |
This makes the code more defensive against edge cases.
|
|
||
| ## Validate Traces in SigNoz | ||
|
|
||
| After running your app and make a few HTTP requests to endpoints such as `http://localhost:3002/`, `http://localhost:3002/health`, `http://localhost:3002/api/data`, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar issue:
| After running your app and make a few HTTP requests to endpoints such as `http://localhost:3002/`, `http://localhost:3002/health`, `http://localhost:3002/api/data`, etc. | |
| After running your app and making a few HTTP requests to endpoints such as `http://localhost:3002/`, `http://localhost:3002/health`, `http://localhost:3002/api/data`, etc. |
Closes: #99