chore: add pageId and applicationSlug to all traces#37219
Conversation
WalkthroughThe changes in this pull request focus on updating the telemetry data generation process within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11679785757. |
…rowserAgent.setCustomAttribute method
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11679872708. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/UITelemetry/generateTraces.ts (1)
28-50: Consider adding TypeScript interface for return type.The function implementation looks good, but adding a type definition would improve maintainability and type safety.
+interface AppParams { + appMode: string; + pageId?: string; + branchName?: string; + applicationSlug?: string; +} -const getAppParams = memoizeOne( +const getAppParams = memoizeOne<(origin: string, pathname: string, search: string) => AppParams>( (origin: string, pathname: string, search: string) => {app/client/src/ce/utils/serviceWorkerUtils.ts (1)
61-91: Consider adding applicationSlug validation.While the implementation is solid, consider adding validation for the applicationSlug format to prevent invalid values from propagating through the system.
export const getApplicationParamsFromUrl = ( urlParams: Pick<URL, "origin" | "pathname" | "search">, ): TApplicationParamsOrNull => { const { origin, pathname, search } = urlParams; const branchName = getSearchQuery(search, "branch"); + + // Validate applicationSlug format if present + const validateSlug = (slug?: string) => { + if (!slug) return undefined; + return /^[a-z0-9-]+$/.test(slug) ? slug : undefined; + }; const matchedBuilder: Match<TMatchResult> = matchBuilderPath(pathname, { end: false, }); const matchedViewer: Match<TMatchResult> = matchViewerPath(pathname); if (matchedBuilder) { return { origin, basePageId: matchedBuilder.params.basePageId, baseApplicationId: matchedBuilder.params.baseApplicationId, branchName, appMode: APP_MODE.EDIT, - applicationSlug: matchedBuilder.params.applicationSlug, + applicationSlug: validateSlug(matchedBuilder.params.applicationSlug), }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/UITelemetry/generateTraces.ts(2 hunks)app/client/src/ce/utils/serviceWorkerUtils.ts(3 hunks)
🔇 Additional comments (5)
app/client/src/UITelemetry/generateTraces.ts (2)
19-19: LGTM! Import changes align with the new implementation.
Line range hint 52-64: Verify telemetry attribute requirements.
The implementation adds the new attributes correctly. However, let's verify if any of these attributes are required for the telemetry system.
✅ Verification successful
Let me broaden the search to find telemetry-related code and attribute requirements. The previous search might have been too specific. These new patterns should help us understand the telemetry implementation and any attribute validation requirements.
Based on the search results, I can now provide a final response regarding the telemetry attributes verification:
No validation required for telemetry attributes
The telemetry implementation uses OpenTelemetry SDK which handles attribute validation internally. The common attributes added in getCommonTelemetryAttributes() are standard system properties that are always available in the browser environment:
deviceType,browserName,browserVersion- Browser detection utilitieshostname- Fromwindow.locationosName,osVersion- OS detection utilitiesotlpSessionId- Generated session ID- Application params from URL
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for telemetry attribute validations or requirements
rg -g '!*.{lock,md}' -A 3 'telemetry.*attributes?' .
Length of output: 54
Script:
#!/bin/bash
# Search for telemetry related code with broader patterns
rg -g '!*.{lock,md}' -A 5 'telemetry|TELEMETRY' .
# Search for attribute usage in trace generation
rg -g '!*.{lock,md}' -A 5 'generateTrace|createTrace|addAttributes' .
# Look for any OpenTelemetry configuration
rg -g '!*.{lock,md}' -A 5 '@opentelemetry|opentelemetry' .
Length of output: 99888
app/client/src/ce/utils/serviceWorkerUtils.ts (3)
17-17: LGTM: Interface changes are consistent and well-structured.
The optional applicationSlug property is appropriately added to both interfaces, maintaining type consistency.
Also applies to: 26-26
61-62: Great use of Pick type for parameter specification.
The change to use Pick makes the function more precise and type-safe by explicitly declaring required URL properties.
64-91: Implementation looks good, but verify error handling.
The changes consistently handle applicationSlug across both builder and viewer paths. However, let's verify error handling for this new parameter.
|
Deploy-Preview-URL: https://ce-37219.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/utils/serviceWorkerUtils.test.ts (1)
Line range hint
215-315: Consider adding edge cases for applicationSlug validation.While the current tests cover the happy path, consider adding test cases for:
- URLs with special characters in applicationSlug
- URLs with very long applicationSlug values
- URLs with empty applicationSlug
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/ce/utils/serviceWorkerUtils.test.ts(3 hunks)
🔇 Additional comments (3)
app/client/src/ce/utils/serviceWorkerUtils.test.ts (3)
215-215: LGTM: Builder path test correctly verifies applicationSlug.
The test case properly validates the extraction of applicationSlug from the builder path URL.
231-231: LGTM: Viewer path test correctly verifies applicationSlug.
The test case properly validates the extraction of applicationSlug from the viewer path URL.
312-312: LGTM: Test verifies applicationSlug extraction without branch parameter.
The test case properly validates that applicationSlug is extracted correctly even when the branch query parameter is absent.
## Description Add pageId and applicationSlug as common attributes to all spans Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/11679770316> > Commit: de60693 > Workflow: `PR Automation test suite` > Tags: `@tag.Sanity` > Spec: `` > <hr>Tue, 05 Nov 2024 07:52:37 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
3 participants
Description
Add pageId and applicationSlug as common attributes to all spans
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11699130143
Commit: d9f4876
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 06 Nov 2024 08:15:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
applicationSlugproperty to key interfaces for improved data handling.Bug Fixes