chore: Refactor Codebase to Remove Telemetry from CE #39421
Conversation
WalkthroughThis PR introduces a new server implementation in the RTS package with a dedicated CE server that mounts various API routes and enhanced logging. The EE server now imports this new CE server, and the legacy RTS server file has been removed. Additionally, multiple Java files have been updated to simplify span management by removing an unnecessary boolean parameter from tracing methods. Several metrics and observability configurations have also been removed or simplified, and new properties files have been added to consolidate server configuration. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (18)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (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
|
subhrashisdas
added
ok-to-test
github-actions
bot
removed
the
skip-changelog
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13498590050. |
![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
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/resources/application-ce.properties (2)
53-55: Verify telemetry settings align with CE requirements.Telemetry is disabled by default (
disable.telemetry = true), but segment key configurations are still present. Consider removing unused telemetry-related properties if they won't be used in CE.-segment.writeKey=${APPSMITH_SEGMENT_KEY:} -segment.ce.key = ${APPSMITH_SEGMENT_CE_KEY:} -logging.verbose.enabled = ${APPSMITH_VERBOSE_LOGGING_ENABLED:false}
58-62: Consider removing Sentry configuration from CE.Since this PR aims to remove telemetry from CE, consider if Sentry error tracking should also be removed.
-sentry.dsn=${APPSMITH_SERVER_SENTRY_DSN:} -sentry.send-default-pii=true -sentry.debug=false -sentry.environment=${APPSMITH_SERVER_SENTRY_ENVIRONMENT:}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/client/packages/rts/src/ce/server.ts(1 hunks)app/client/packages/rts/src/ee/server.ts(1 hunks)app/client/packages/rts/src/instrumentation.ts(0 hunks)app/client/packages/rts/src/server.ts(1 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(4 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java(5 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/ObservationHelper.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ReactorNettyConfiguration.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ObservationHelperImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/resources/application-ce.properties(1 hunks)app/server/appsmith-server/src/main/resources/application-ee.properties(1 hunks)app/server/appsmith-server/src/main/resources/application.properties(1 hunks)
💤 Files with no reviewable changes (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java
- app/client/packages/rts/src/instrumentation.ts
✅ Files skipped from review due to trivial changes (5)
- app/client/packages/rts/src/ee/server.ts
- app/server/appsmith-server/src/main/resources/application-ee.properties
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
- app/client/packages/rts/src/server.ts
- app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java
👮 Files not reviewed due to content moderation or server errors (2)
- app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/ObservationHelper.java
🧰 Additional context used
🧠 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (2)
Learnt from: rishabhrathod01
PR: appsmithorg/appsmith#37001
File: app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java:149-150
Timestamp: 2024-11-12T08:11:36.416Z
Learning: In the `UpdateLayoutServiceCEImpl` class in `UpdateLayoutServiceCEImpl.java`, when the `Span` starts before the `try-catch` and ends after the `try-catch` block, there's no need to add `endSpan` in a `finally` block unless `startSpan` is called within the `try` block.
Learnt from: rishabhrathod01
PR: appsmithorg/appsmith#37001
File: app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java:136-138
Timestamp: 2024-11-12T08:11:36.416Z
Learning: When a `Span` is started before a `try-catch` block and ended after it, there is no need to end the span inside the catch block.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (14)
app/client/packages/rts/src/ce/server.ts (2)
25-25: LGTM! Good security practice.The
x-powered-byheader is properly disabled to prevent information disclosure.
35-36: Verify timeout values are sufficient for your use case.The header timeout (61s) and keep-alive timeout (60s) values are set to standard values, but verify these are sufficient for your application's needs.
app/server/appsmith-server/src/main/resources/application.properties (1)
1-1: LGTM! Clean separation of configurations.Good modularization by moving configurations to separate CE/EE property files.
app/server/appsmith-server/src/main/resources/application-ce.properties (1)
105-105: Verify secure handling of internal password.Ensure the internal password is properly encrypted/hashed when stored and transmitted.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ObservationHelperImpl.java (1)
47-49: LGTM! Simplified span management.The removal of the
isDetailparameter and associated conditional logic makes the code cleaner and more maintainable while preserving the core functionality.Also applies to: 52-54
app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (1)
138-138: LGTM! Consistent with simplified span management.The span management calls have been updated correctly to match the new method signatures.
Also applies to: 149-149
app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (1)
231-231: LGTM! Aligned with simplified span management.The span management calls have been updated correctly to match the new method signatures.
Also applies to: 239-239
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ReactorNettyConfiguration.java (1)
18-18: LGTM! Simplified metrics configuration.The metrics configuration has been simplified by removing conditional checks, making it consistent with the overall telemetry removal changes.
app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java (3)
86-86: LGTM! Clean removal of telemetry details.The changes consistently remove the
isDetailparameter from span management calls across all methods while maintaining the core functionality.Also applies to: 94-94, 106-106, 112-112, 125-125, 134-134, 292-292, 299-299
86-86: LGTM! Consistent simplification of span tracking.The changes align with the PR objectives by removing unnecessary telemetry parameters across all methods.
Also applies to: 94-94, 106-106, 112-112, 125-125, 134-134, 292-292, 299-299
86-86: LGTM! Consistent removal of unused telemetry parameter.The changes align with the PR objectives by simplifying telemetry-related code. The modifications are consistent across all methods, and the functionality remains unchanged.
Also applies to: 94-94, 106-106, 112-112, 125-125, 134-134, 292-292, 299-299
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/ObservationHelper.java (3)
10-12: LGTM! Clean interface simplification.The interface has been streamlined by removing the unused
isDetailparameter while maintaining backward compatibility.Also applies to: 14-14
10-12: LGTM! Clean interface simplification.The removal of the unused
isDetailparameter streamlines the interface while maintaining functionality.Also applies to: 14-14
10-12: LGTM! Interface simplified by removing unused parameter.The interface changes are consistent with the implementation changes and align with the PR objectives.
Also applies to: 14-14
subhrashisdas
changed the title
github-actions
bot
added
the
skip-changelog
subhrashisdas
force-pushed
the
feature/telemetry-ce
branch
from
6d4f98b to
79794e5
Compare
subhrashisdas
added
ok-to-test
rajatagrawal
added
ok-to-test
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
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/13515594569
Commit: 79794e5
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 25 Feb 2025 07:48:37 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor / Chores