-
Notifications
You must be signed in to change notification settings - Fork 129
chore: update frontend monitoring docs #2151
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
7b1fd89 to
de85556
Compare
d7eaa02 to
71233b6
Compare
|
@gemini-cli /review |
|
🤖 Hi @makeavish, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@claude /review |
|
Claude finished @makeavish's task —— View job Review SummaryI've reviewed the frontend monitoring documentation updates against the guidelines in CONTRIBUTING.md. Overall, the PR successfully achieves its goals of making the docs framework-agnostic and updating to ES2020 syntax. Here's my assessment: ✅ Strengths
|
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.
📋 Docs Review Summary
- Scope: documentation-only review. Non-docs changes were ignored.
- The PR updates the frontend monitoring docs to make them framework-agnostic and align them with the latest conventions. The changes are well-structured and provide clear instructions for different frameworks.
✅ PR Checklist Reference (from CONTRIBUTING.md)
- The PR follows most of the guidelines from the
CONTRIBUTING.mdfile. - I have suggested some minor changes to align the PR with the contribution guidelines.
🔍 General Feedback
- The use of placeholders is inconsistent with the
CONTRIBUTING.mdfile. I have added comments to fix this. - Some code snippets are missing semicolons at the end of lines. I have added comments to fix this.
- The files do not end with a newline. I have added comments to fix this.
|
🤖 Hi @amlannandy, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
Claude finished @amlannandy's task —— View job Documentation Review CompleteI've reviewed the frontend monitoring documentation updates against CONTRIBUTING.md guidelines and created inline comments for specific issues. Todo List:
SummaryThe PR successfully achieves its main goals of making the docs framework-agnostic with excellent tab implementations for React, Next.js, Nuxt, Angular, Vue, and Svelte. The structure is consistent across all three files with good code examples using ES2020 syntax. Issues IdentifiedI've created 6 inline comments on specific code issues. Here's the breakdown: Critical Issues (Block Merge):
Code Issues:
PR Checklist AssessmentBased on CONTRIBUTING.md "Docs PR Checklist" (lines 315-332):
RecommendationsMust Fix (per CONTRIBUTING.md):
Nice to Have:
Overall AssessmentThis is a significant improvement to the frontend monitoring docs. The framework-agnostic approach with tabs is exactly what users need. However, the placeholder format violations and missing troubleshooting sections need to be addressed before merge as they are explicit requirements in CONTRIBUTING.md. The inline comments provide specific fixes for code issues. The troubleshooting sections need to be added manually to all three files. |
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.
📋 Docs Review Summary
- Scope: documentation-only review. Non-docs changes were ignored.
- This pull request significantly improves the frontend monitoring documentation by adding framework-specific examples and clarifying the setup process. The changes make the documentation more accessible and easier to follow for a wider range of users. The overall structure is more logical and aligns better with our standards.
✅ PR Checklist Reference (from CONTRIBUTING.md)
- Frontmatter: Pass. The frontmatter now includes a
description, which is a great addition for SEO and clarity. - Placeholders: Needs fixes. The placeholders have been changed from
<PLACEHOLDER>to<<PLACEHOLDER>>, which is inconsistent with ourCONTRIBUTING.md. I've added a comment to address this. This applies to all three files in this PR. - Commands: Needs fixes. The
npm installandyarn addcommands were changed to a single line, which reduces readability. I've added a comment suggesting a return to the multi-line format. This applies to all three files in this PR. - Content Structure: Pass. The new structure with framework-specific tabs is a major improvement.
🔍 General Feedback
- The use of
entityName="plans"in the<Tabs>component is noted. While not explicitly inCONTRIBUTING.md, it appears to be a valid and useful feature for creating distinct tab groups. - The addition of
instrumentation.tsand framework-specific import instructions is a fantastic improvement that will help users get started much more quickly.
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.
- CIs are failing please check
- Note saying
This step is only needed when using NextJs 14 and belowis unclear. Update it to mentioninstrumentationHook config in next.config.js only needs to enabled in NextJs versions 14 or below - Condition in NextJs section seems to be missing:

- We should remove example code for backend as it lengthens doc without adding any value. Any code post this line -
For the purpose of this guide, we have setup a simple Node.js API with instrumentation. - This line -
import { SpanProcessor } from '@opentelemetry/sdk-trace-web';is missing for .js, is it intentional? - Zone context manager - #1796 (comment)
- Will the instrumentation work for Nextjs when someone already is using vercel/otel? - https://signoz-web-git-chore-update-frontend-docs-signoz.vercel.app/docs/instrumentation/opentelemetry-nextjs/
- Will the instrumetation work for Nuxtjs work for users who have already instrumented following this doc - https://signoz-web-git-chore-update-frontend-docs-signoz.vercel.app/docs/instrumentation/opentelemetry-nuxtjs/
Should we have a single separate doc for Nextjs and Nuxtjs as they are full stack frameworks?
ca61712 to
3199909
Compare
|
@makeavish Addressed points 1-6.
|
Uh oh!
There was an error while loading. Please reload this page.