-
Notifications
You must be signed in to change notification settings - Fork 114
chore: update web vitals monitoring doc #1796
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
|
[Resolved] [vc]: #NNm20yoXwh3qcK8qfKX6ulNIuJJL+dGVg1dcALuzvTw=:eyJpc01vbm9yZXBvIjp0cnVlLCJ0eXBlIjoiZ2l0aHViIiwicHJvamVjdHMiOlt7Im5hbWUiOiJzaWdub3otd2ViIiwiaW5zcGVjdG9yVXJsIjoiaHR0cHM6Ly92ZXJjZWwuY29tL3NpZ25vei9zaWdub3otd2ViL0d0VTRRUnhOS3RGZUtOWFZHMzNTWUtrQ3R3SEUiLCJwcmV2aWV3VXJsIjoic2lnbm96LXdlYi1naXQtc2lnLTI2Njgtc2lnbm96LnZlcmNlbC5hcHAiLCJuZXh0Q29tbWl0U3RhdHVzIjoiREVQTE9ZRUQiLCJsaXZlRmVlZGJhY2siOnsicmVzb2x2ZWQiOjAsInVucmVzb2x2ZWQiOjAsInRvdGFsIjowLCJsaW5rIjoic2lnbm96LXdlYi1naXQtc2lnLTI2Njgtc2lnbm96LnZlcmNlbC5hcHAifSwicm9vdERpcmVjdG9yeSI6bnVsbH1dfQ==
|
9bd1705 to
e67b416
Compare
data/docs/dashboards/dashboard-templates/frontend-monitoring.mdx
Outdated
Show resolved
Hide resolved
|
Claude finished @amlannandy's task —— View job Documentation Review CompleteI've completed a comprehensive review of the web vitals documentation changes according to CONTRIBUTING.md standards and addressed Makeavish's feedback. SummaryThis PR successfully restructures the web vitals documentation by:
The technical content is accurate (Core Web Vitals verified against official web.dev sources), and the structure is excellent. However, there are critical issues that must be fixed before merging, plus important improvements needed to address Makeavish's feedback. Critical Issues (Must Fix)1. Incorrect
2. ID/filename mismatch in overview (opentelemetry-web-vitals.mdx:5)
High Priority Issues (Address Makeavish's Feedback)3. Missing comprehensive Next Steps sections (Both guides, lines 503/520)
4. Clarity for existing frontend monitoring users (Both guides, line 14)
Technical Accuracy Verified ✅Core Web Vitals are correctly documented across all files:
Source: https://web.dev/articles/vitals Review Against CONTRIBUTING.md PR Checklist
What's Working Well
Inline Comments PostedI've posted 5 inline comments with specific recommendations:
RecommendationThe critical ID issues (#1, #2) must be fixed before merging as they will cause routing problems and conflicts. The Next Steps and clarity improvements (#3, #4) are high priority to address Makeavish's valid concerns about usability for existing monitoring users. After these fixes, the documentation will be in excellent shape and ready for merge. |
data/docs/dashboards/dashboard-templates/frontend-monitoring.mdx
Outdated
Show resolved
Hide resolved
|
Web vitals traces doc mentions contextManager: new ZoneContextManager() in step 2, which is not mentioned in traces frontend monitoring. But we are asking users to skip step 1 and 2 if already followed frontend monitoring @amlannandy |
|
Claude finished @amlannandy's task —— View job Documentation Inconsistency AnalysisI've investigated @makeavish's concern about the ZoneContextManager documentation inconsistency. Todo List:
Issue ConfirmedThe Problem: The web vitals traces doc (
However, there's a critical difference between the two approaches: Web Vitals Traces Doc (lines 97-99): provider.register({
contextManager: new ZoneContextManager(),
});Main Frontend Traces Doc ( provider.register();The main frontend traces documentation does NOT include Why This MattersThe RecommendationsOption 1 (Recommended): Update the main frontend traces documentation ( import { ZoneContextManager } from '@opentelemetry/context-zone';
// ... existing code ...
provider.register({
contextManager: new ZoneContextManager(),
});This makes the "skip steps 1-2" statement accurate and improves the main traces guide. Option 2: Remove or modify the "skip steps 1-2" guidance in **Already sending frontend traces?** If you've followed the Sending Traces guide, you can skip Step 1, but you should review Step 2 as it includes `ZoneContextManager` configuration which may differ from your existing setup.Option 3: Add a note in the web vitals traces doc explicitly calling out the ZoneContextManager difference for users who followed the main traces guide. Additional ContextAccording to OpenTelemetry documentation, Source: https://opentelemetry.io/docs/languages/js/context/#context-managers Would you like me to implement any of these fixes? |
Okay then the sending-traces doc should also have it. I will update that doc in the other PR that we have to include the ZoneContextManager |
|
Claude finished @amlannandy's task —— View job Documentation Review Complete ✅I've completed a comprehensive review of the web vitals documentation changes against CONTRIBUTING.md standards. SummaryThis PR successfully restructures the web vitals documentation by:
The technical content is accurate and follows OpenTelemetry best practices. Core Web Vitals (LCP, INP, CLS, TTFB, FCP) are correctly documented per Google's 2025 standards (Source: https://web.dev/articles/vitals). Review Against CONTRIBUTING.md PR Checklist✅ Frontmatter: Complete with Technical Accuracy Verified ✅Core Web Vitals are correctly documented across all files:
Additional Web Vitals:
Source: https://web.dev/articles/vitals All OpenTelemetry configurations follow current best practices for web instrumentation. Status of Previous Review IssuesComparing with all previous Claude reviews (Nov 4-5), the following have been successfully resolved: ✅ Tags corrected to Addressing @makeavish's Feedback1. ZoneContextManager Inconsistency (Resolved)@makeavish noted that the web vitals traces doc uses Status: @amlannandy confirmed this will be addressed in a separate PR by adding 2. Missing Next StepsStatus: ✅ Resolved. Both guides now include Next Steps sections:
3. Clarity for Existing Frontend Monitoring UsersStatus: ✅ Well-addressed. Both guides include prominent Admonition blocks at the top (lines 13-15 in each) that clearly explain:
Minor Observations (Non-blocking)
What's Working Exceptionally Well
RecommendationThis PR is ready for merge. The documentation successfully addresses all critical issues from previous reviews and properly implements feedback from @makeavish. The restructuring significantly improves clarity and provides excellent guidance for implementing web vitals monitoring with SigNoz. The minor observations about link format and component props are style preferences that don't impact functionality or user experience. Files Reviewed
Overall Assessment: This is a significant improvement to the SigNoz documentation. The restructuring makes it much easier for users to understand and implement web vitals monitoring, with clear guidance on when to use metrics vs traces and excellent support for users with existing instrumentation. No blocking issues found. |
Update docs to use traces for capturing web vitals