Conversation
WalkthroughThe changes introduce a telemetry feature to the Axios instance in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/api/Api.ts (3)
29-29: Fix typo in variable nameThe variable name has a typo:
transfromFnshould betransformFn- const transfromFn = transformResponseAr?.[0]; + const transformFn = transformResponseAr?.[0];
38-42: Enhance error loggingThe error logging could be more informative by including the response data type and size.
- console.error("could not find the api transformerFn"); + console.error("API transform function not found", { + dataType: typeof args[0], + dataSize: args[0]?.length, + });
23-45: Consider performance implications of transform wrappingWhile adding telemetry is valuable, wrapping every response transformation might impact performance for high-frequency API calls. Consider:
- Sampling the telemetry data
- Adding a threshold for logging only slow transformations
- Adding the ability to disable this in production if needed
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/api/Api.ts (2 hunks)
🔇 Additional comments (2)
app/client/src/api/Api.ts (2)
10-10: LGTM: Clean import additionThe telemetry function import is well-placed and appropriately named.
32-32: Verify URL context availabilityThe
this.urlmight be undefined in some contexts. Consider adding a fallback.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11511864501. |
|
Deploy-Preview-URL: https://ce-37081.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11512035187. |
|
Deploy-Preview-URL: https://ce-37081.dp.appsmith.com |
## Description Capture performance of parsing api responses. 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.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11512264036> > Commit: 92f9d88 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11512264036&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Fri, 25 Oct 2024 06:47:52 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a telemetry feature for enhanced monitoring of API requests and responses. - **Bug Fixes** - Improved error handling by logging issues when default transformation functions are not found. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Capture performance of parsing api responses.
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.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11512264036
Commit: 92f9d88
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 25 Oct 2024 06:47:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes