chore: add grafana faro sdk (CE) #38301
Conversation
dvj1988
requested review from
sharat87,
pratapaprasanna,
ApekshaBhosale,
ayushpahwa,
a team and
KelvinOm
as code owners
WalkthroughThe pull request introduces a migration from Sentry and New Relic to Grafana Faro for observability and telemetry across the client application. Key changes include the addition of Faro-related environment variables, updates to Webpack plugins, modifications to package dependencies, and enhancements to tracing mechanisms. The focus is on improving application monitoring and performance tracking using Grafana's observability platform. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 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
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs (1)
249-249: Ensure environment variable consistency
Make sure the environment variable name (APPSMITH_HOSTNAME) is consistent everywhere and doesn’t introduce unexpected overrides or collisions with other HOSTNAME references.app/client/src/index.tsx (1)
31-31: Consider removing or renaming references to New Relic if no longer needed.
This block still references New Relic configurations, which might be deprecated or superseded by the new telemetry framework (Grafana Faro).app/client/src/instrumentation/generateTraces.ts (1)
101-106: startNestedSpan uses generateContext.
Good approach for parent-child spans.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (60)
.github/workflows/client-build.yml(2 hunks)app/client/craco.build.config.js(2 hunks)app/client/knip.json(1 hunks)app/client/package.json(2 hunks)app/client/public/index.html(1 hunks)app/client/src/UITelemetry/auto-otel-web.ts(0 hunks)app/client/src/actions/pluginActionActions.ts(2 hunks)app/client/src/api/Api.ts(1 hunks)app/client/src/ce/plugins/Linting/utils/getEntityUniqueIdForLogs.ts(1 hunks)app/client/src/ce/workers/Evaluation/evaluationUtils.test.ts(1 hunks)app/client/src/ce/workers/Evaluation/getEntityForEvalContextMap.ts(1 hunks)app/client/src/ce/workers/common/DependencyMap/utils/getEntityDependenciesByType.ts(1 hunks)app/client/src/ce/workers/common/types.ts(2 hunks)app/client/src/components/formControls/utils.ts(1 hunks)app/client/src/entities/DataTree/dataTreeFactory.ts(1 hunks)app/client/src/entities/Engine/AppEditorEngine.ts(1 hunks)app/client/src/entities/Engine/AppViewerEngine.ts(1 hunks)app/client/src/entities/Engine/index.ts(1 hunks)app/client/src/index.tsx(2 hunks)app/client/src/instrumentation/generateTraces.ts(7 hunks)app/client/src/instrumentation/generateWebWorkerTraces.ts(5 hunks)app/client/src/instrumentation/index.ts(1 hunks)app/client/src/instrumentation/types.ts(1 hunks)app/client/src/plugins/Linting/types.ts(1 hunks)app/client/src/plugins/Linting/utils/getLintingErrors.ts(1 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts(1 hunks)app/client/src/sagas/EvalWorkerActionSagas.ts(1 hunks)app/client/src/sagas/EvaluationsSaga.ts(1 hunks)app/client/src/sagas/InitSagas.ts(1 hunks)app/client/src/sagas/JSLibrarySaga.ts(1 hunks)app/client/src/sagas/PostEvaluationSagas.ts(1 hunks)app/client/src/sagas/__tests__/initSagas.test.ts(1 hunks)app/client/src/utils/WorkerUtil.ts(1 hunks)app/client/src/utils/helpers.tsx(1 hunks)app/client/src/widgets/BaseWidgetHOC/WidgetProfiler.tsx(1 hunks)app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts(1 hunks)app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx(1 hunks)app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts(1 hunks)app/client/src/widgets/JSONFormWidget/widget/index.tsx(1 hunks)app/client/src/widgets/withWidgetProps.tsx(1 hunks)app/client/src/workers/Evaluation/JSObject/__test__/mutation.test.ts(1 hunks)app/client/src/workers/Evaluation/JSObject/utils.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/Actions.test.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/evaluate.test.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/evaluation.test.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/evaluationSubstitution.test.ts(1 hunks)app/client/src/workers/Evaluation/__tests__/timeout.test.ts(1 hunks)app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts(1 hunks)app/client/src/workers/Evaluation/evaluationSubstitution.ts(1 hunks)app/client/src/workers/Evaluation/fns/__tests__/index.test.ts(1 hunks)app/client/src/workers/Evaluation/fns/__tests__/interval.test.ts(1 hunks)app/client/src/workers/Evaluation/handlers/evalTree.ts(2 hunks)app/client/src/workers/Evaluation/types.ts(2 hunks)app/client/src/workers/common/DataTreeEvaluator/index.ts(6 hunks)app/client/src/workers/common/DataTreeEvaluator/mockData/ArrayAccessorTree.ts(1 hunks)app/client/src/workers/common/DataTreeEvaluator/mockData/NestedArrayAccessorTree.ts(1 hunks)app/client/src/workers/common/DataTreeEvaluator/mockData/mockConfigTree.ts(1 hunks)app/client/src/workers/common/DataTreeEvaluator/mockData/mockUnEvalTree.ts(1 hunks)app/client/src/workers/common/DependencyMap/index.ts(1 hunks)deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/UITelemetry/auto-otel-web.ts
✅ Files skipped from review due to trivial changes (28)
- app/client/src/entities/Engine/AppViewerEngine.ts
- app/client/src/widgets/BaseWidgetHOC/WidgetProfiler.tsx
- app/client/src/sagas/InitSagas.ts
- app/client/src/widgets/JSONFormWidget/widget/index.tsx
- app/client/src/sagas/tests/initSagas.test.ts
- app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.ts
- app/client/src/plugins/Linting/utils/getLintingErrors.ts
- app/client/src/entities/Engine/AppEditorEngine.ts
- app/client/src/entities/Engine/index.ts
- app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts
- app/client/src/sagas/EvaluationsSaga.ts
- app/client/src/widgets/withWidgetProps.tsx
- app/client/src/workers/Evaluation/evaluationSubstitution.ts
- app/client/src/api/Api.ts
- app/client/src/sagas/PostEvaluationSagas.ts
- app/client/src/sagas/EvalWorkerActionSagas.ts
- app/client/src/utils/helpers.tsx
- app/client/src/workers/Evaluation/tests/evaluation.test.ts
- app/client/src/entities/DataTree/dataTreeFactory.ts
- app/client/src/workers/common/DataTreeEvaluator/mockData/ArrayAccessorTree.ts
- app/client/src/components/formControls/utils.ts
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts
- app/client/src/workers/Evaluation/tests/evaluationSubstitution.test.ts
- app/client/src/workers/common/DataTreeEvaluator/mockData/mockUnEvalTree.ts
- app/client/src/utils/WorkerUtil.ts
- app/client/src/sagas/JSLibrarySaga.ts
- app/client/src/ce/workers/Evaluation/evaluationUtils.test.ts
- app/client/src/ce/workers/common/DependencyMap/utils/getEntityDependenciesByType.ts
🔇 Additional comments (55)
app/client/public/index.html (1)
217-217: Fallback logic looks good
Referencing the new APPSMITH_HOSTNAME environment variable along with a default of "appsmith-0" provides a clear fallback mechanism for service identification.
app/client/src/widgets/JSONFormWidget/fields/useUnmountFieldValidation.test.tsx (1)
4-4:
Fix mismatch in import vs. jest.mock paths
Currently, the import references "instrumentation/generateTraces", but the jest.mock statement below still points to the old "UITelemetry/generateTraces" path. This mismatch will break coverage or lead to failing tests.
Here’s the suggested fix:
-jest.mock("UITelemetry/generateTraces", () => ({
+jest.mock("instrumentation/generateTraces", () => ({
startAndEndSpanForFn: jest.fn((name, options, fn) => fn()),
}));⛔ Skipped due to learnings
Learnt from: rahulbarwal
PR: appsmithorg/appsmith#37289
File: app/client/src/widgets/JSONFormWidget/fields/SelectField.test.tsx:137-138
Timestamp: 2024-11-12T08:11:25.417Z
Learning: In TypeScript test files like `SelectField.test.tsx`, it's acceptable to use the `delete` operator to remove mock objects such as `ResizeObserver` when their eventual removal is required.
app/client/src/instrumentation/index.ts (5)
1-21: Imports look consistent and well-organized.
They clearly separate the OpenTelemetry and Grafana Faro dependencies.
28-43: Ensure correct Faro collector URL and usage.
Confirm that the endpoint and app details in the initializeFaro configuration are correct for your deployment.
45-51: Validate resource parameters.
Make sure that deploymentName, serviceInstanceId, and serviceName accurately reflect your environment for accurate telemetry.
53-59: Span processing setup looks good.
The chained processors and exporter align with standard practices for exporting traces.
64-64: Nice initialization of OTEL with Faro.
This final step ensures Faro receives trace context properly.
app/client/craco.build.config.js (2)
7-7: Importing FaroSourceMapUploaderPlugin is correct.
It replaces the removed Sentry plugin neatly.
42-53: Validate required environment variables.
Ensure that REACT_APP_FARO_* variables are set to avoid source map upload failures.
app/client/src/workers/common/DataTreeEvaluator/index.ts (3)
35-38: Updated imports from "ee/entities/DataTree/types" are consistent.
Aligning entity imports in one place improves maintainability.
147-147: Instrumentations references look fine.
Including WebworkerSpanData and Attributes fosters consistent telemetry.
Also applies to: 157-158
257-257: webworkerTelemetry usage is clear.
Capturing and recording span data within the worker helps unify observability.
Also applies to: 555-555, 639-639
app/client/src/instrumentation/types.ts (1)
1-8: New WebworkerSpanData interface.
This addition cleanly structures span data for web worker contexts.
app/client/src/ce/workers/Evaluation/getEntityForEvalContextMap.ts (1)
1-1: Import path update for ENTITY_TYPE is valid.
Centralizing the import from "ee/entities/DataTree/types" matches project consistency.
app/client/src/ce/workers/common/types.ts (2)
1-2: Imports are consistent with the new instrumentation module.
This switch aligns well with the updated OpenTelemetry approach.
16-16: Union type usage for telemetry is clear.
Using a union type for storing either raw attributes or custom span data is a flexible approach.
app/client/src/ce/plugins/Linting/utils/getEntityUniqueIdForLogs.ts (1)
2-2: Updated import path for ENTITY_TYPE confirmed.
The revised import is aligned with the new “ee/entities” structure. No action needed.
app/client/src/instrumentation/generateWebWorkerTraces.ts (3)
1-3: Separation of Span & Attributes imports looks good.
This clarifies the usage of OpenTelemetry data types, removing ambiguity between custom vs. built-in definitions.
10-10: Use of 'Attributes' defaults ensures extensibility.
Having default empty objects for optional attributes is a good practice, making function calls simpler.
Also applies to: 27-28, 41-42
56-56: Parent span usage ensures correct trace hierarchy.
Wrapping child spans within a parentSpan context is consistent with OpenTelemetry recommended patterns.
Also applies to: 70-70
app/client/src/plugins/Linting/types.ts (1)
14-15: Telemetry imports reorganized for clarity.
Switching to a single “instrumentation” source centralizes tracing logic and avoids scattered imports.
app/client/src/workers/Evaluation/fns/__tests__/index.test.ts (1)
5-5: Looks Good.
The new import path aligns with the broader codebase reorganization.
app/client/src/index.tsx (1)
46-46: Dynamically loading instrumentation module.
Good use of code splitting for loading the instrumentation script. Ensure all references to this instrumentation code are updated.
app/client/src/workers/Evaluation/types.ts (2)
18-21: Consistent usage of OpenTelemetry types.
Switching from custom definitions to standardized OpenTelemetry “Attributes” helps maintain consistency and clarity.
71-71: Verify extended usage of telemetries.
Check if all consumers of “webworkerTelemetry” correctly handle the new Attributes type, especially those expecting the old “SpanAttributes.”
✅ Verification successful
Let me analyze the usage of webworkerTelemetry with the new Attributes type. From the search results, I can see all the places where it's used, but let me verify the type definitions and usage patterns to ensure compatibility.
Let me try another search to find the type definitions and usage patterns.
Let me try one more search with corrected syntax to find the type definitions and usage.
Based on the search results, I can see that webworkerTelemetry is used with both WebworkerSpanData and Attributes types, which are imported from "@opentelemetry/api". The imports and usage patterns show that the consumers are already handling both types correctly through the union type.
No issues found with telemetry type handling.
The codebase already supports both WebworkerSpanData and Attributes types in telemetry consumers, with proper type imports and consistent usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to webworkerTelemetry usage
rg -A 5 "webworkerTelemetry"
Length of output: 48633
Script:
#!/bin/bash
# Search for WebworkerSpanData and Attributes type definitions
ast-grep --pattern 'type WebworkerSpanData = $_'
ast-grep --pattern 'type Attributes = $_'
ast-grep --pattern 'interface WebworkerSpanData $_'
ast-grep --pattern 'interface Attributes $_'
Length of output: 76693
Script:
#!/bin/bash
# Search for type definitions and imports
rg -A 3 "type (WebworkerSpanData|Attributes|SpanAttributes)"
rg -A 3 "import.*{.*(WebworkerSpanData|Attributes|SpanAttributes).*}"
Length of output: 285
Script:
#!/bin/bash
# Search for type definitions and imports with fixed regex
rg "type (WebworkerSpanData|Attributes|SpanAttributes)"
rg "import.*WebworkerSpanData"
rg "import.*Attributes"
rg "import.*SpanAttributes"
Length of output: 68477
app/client/src/workers/Evaluation/JSObject/__test__/mutation.test.ts (1)
2-2: Import path updated to the EE module.
Ensure consistency across CE and EE references in the rest of the file to avoid confusion.
app/client/src/workers/Evaluation/__tests__/timeout.test.ts (1)
4-4: Import path looks good.
No issues with the new import. This update aligns with the refactor in the codebase.
app/client/src/instrumentation/generateTraces.ts (10)
8-9: Renamed imports from OpenTelemetry.
Renaming to OTEL_CONTEXT and OTEL_TRACE for clarity is acceptable.
20-26: Usage of faro's OTEL fallback.
Ensure the fallback object is valid; otherwise, references to trace or context could be undefined.
28-28: DEFAULT_TRACE constant is clear.
Naming is consistent and descriptive.
74-80: startRootSpan changes.
Switching to Attributes is consistent with OpenTelemetry usage.
91-95: generateContext fallback logic.
Bypassing context if undefined is guarded properly.
118-118: Return statement.
Returning the span is essential for further usage.
127-127: setAttributesToSpan.
Straightforward usage.
134-140: startAndEndSpanForFn usage.
Updating spanAttributes to Attributes is consistent with other changes.
149-155: startAndEndSpan updates.
Ensuring accurate endTime is helpful for more precise tracing.
158-171: convertWebworkerSpansToRegularSpans function.
Iterating and creating nested spans for each record is a nice extension of the tracing logic.
app/client/src/workers/Evaluation/fns/__tests__/interval.test.ts (1)
7-7: Import path updated.
This aligns with the refactor to centralize entity-type definitions.
app/client/src/actions/pluginActionActions.ts (2)
24-24: Telemetry type import from OpenTelemetry.
Switching from a custom OtlpSpan type to the official Span type is a good standardization step.
400-404: Update parameter types for parentSpan.
Using the OpenTelemetry Span type improves consistency across the codebase.
app/client/src/workers/common/DataTreeEvaluator/mockData/mockConfigTree.ts (1)
2-2: Import path update looks good
No functional impact observed.
app/client/src/workers/Evaluation/handlers/evalTree.ts (3)
33-33: Usage of updated instrumentation path
This new import appears correct and aligns with the broader telemetry refactor.
36-36: Importing Attributes from @opentelemetry/api
This change is consistent with OpenTelemetry guidelines.
97-97: Attribute assignment for firstEvaluation
Looks fine. Verify that this attribute is correctly consumed or logged downstream.
app/client/src/workers/Evaluation/JSObject/utils.ts (1)
10-10: Consolidated import for EvaluationSubstitutionType
The revised import path improves maintainability by consolidating references to "ee/entities/DataTree/types".
app/client/src/workers/common/DependencyMap/index.ts (2)
38-40: Shifting to new instrumentation traces
These updated imports from "instrumentation/" and "@opentelemetry/api" are consistent with the refactor and look correct.
47-47: Expanded type for webworkerSpans
Ensure callers pass data conforming to the new union type of WebworkerSpanData | Attributes.
app/client/src/workers/Evaluation/__tests__/Actions.test.ts (1)
3-3: No issues with updated import path.
Looks good. This change is consistent with the new module restructuring.
app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts (1)
4-4: Import path update appears orderly.
Keeps the code aligned with the refactor, no further changes needed.
app/client/src/workers/Evaluation/__tests__/evaluate.test.ts (1)
8-8: Upgrade to the new import location is fine.
Everything seems in order.
app/client/src/workers/common/DataTreeEvaluator/mockData/NestedArrayAccessorTree.ts (1)
12-12: Refactor import path to enterprise location is verified.
No code issues spotted.
app/client/knip.json (1)
49-49: LGTM: Dependency configuration is correct
The addition of @grafana/faro-webpack-plugin to ignoreDependencies is consistent with the migration to Grafana Faro.
.github/workflows/client-build.yml (1)
169-173: Environment variables are properly configured
The Faro configuration variables are correctly added and sourced from GitHub secrets.
app/client/package.json (2)
71-72: Runtime dependencies are properly configured
The Faro runtime packages are correctly added with consistent versions (^1.12.2).
256-256: Build dependency is properly configured
The Faro webpack plugin is correctly added as a devDependency.
| REACT_APP_FARO_APP_NAME=${{ secrets.REACT_APP_FARO_APP_NAME }} \ | ||
| REACT_APP_FARO_SOURCEMAP_UPLOAD_API_KEY=${{ secrets.REACT_APP_FARO_SOURCEMAP_UPLOAD_API_KEY }} \ | ||
| REACT_APP_FARO_SOURCEMAP_UPLOAD_ENDPOINT=${{ secrets.REACT_APP_FARO_SOURCEMAP_UPLOAD_ENDPOINT }} \ | ||
| REACT_APP_FARO_STACK_ID=${{ secrets.REACT_APP_FARO_STACK_ID }} \ | ||
| REACT_APP_FUSIONCHARTS_LICENSE_KEY=${{ secrets.APPSMITH_FUSIONCHARTS_LICENSE_KEY }} \ | ||
| SENTRY_AUTH_TOKEN=${{ secrets.SENTRY_AUTH_TOKEN }} \ | ||
| REACT_APP_VERSION_EDITION="Community" \ |
There was a problem hiding this comment.
💡 Codebase verification
Clean up Sentry configuration and implementation
Both Sentry and Grafana Faro instrumentation code are present in the codebase. While Grafana Faro is being implemented (instrumentation/index.ts), Sentry is still actively used with a full implementation in sentry.ts and the SENTRY_AUTH_TOKEN is used in the build workflow. To maintain a clean codebase:
- Remove
SENTRY_AUTH_TOKENfrom the build workflow - Clean up
app/client/src/utils/Analytics/sentry.tsand related imports - Update any remaining Sentry configurations in the app configs
🔗 Analysis chain
Consider cleaning up Sentry configuration
The SENTRY_AUTH_TOKEN environment variable is still present while migrating to Grafana Faro.
The previous search was limited to YAML files, but Sentry configurations could be present in other file types. Let's broaden our search to verify if there are any remaining Sentry-related configurations or code before making a conclusion.
Let's verify if there are any remaining Sentry configurations and check for the Grafana Faro migration to make a more informed decision.
Let's examine the actual implementation files to understand if Sentry is still actively used or if it's been fully migrated to Grafana Faro.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for remaining Sentry configurations
rg -l 'SENTRY|sentry' --type yaml
Length of output: 35
Script:
#!/bin/bash
# Let's check for Sentry-related configurations in all files
rg -l 'SENTRY|sentry'
# Also check for any Sentry imports or usage in JavaScript/TypeScript files
ast-grep --pattern 'import $_ from "@sentry/$_"'
Length of output: 71494
Script:
#!/bin/bash
# Let's check for Sentry-related code and configurations in the client codebase
rg -l 'SENTRY|sentry' app/client/src/utils/Analytics/sentry.ts
rg -l 'SENTRY|sentry' app/client/src/configs/
rg -l 'SENTRY|sentry' app/client/.env*
# Check for Grafana Faro related code
rg -l 'GRAFANA_FARO|grafana.faro|@grafana/faro' app/client/
Length of output: 518
Script:
#!/bin/bash
# Let's check the content of sentry.ts and instrumentation files
rg -A 5 'SENTRY|sentry' app/client/src/utils/Analytics/sentry.ts
rg -A 5 '@grafana/faro' app/client/src/instrumentation/index.ts
# Check if SENTRY_AUTH_TOKEN is used in any workflow files
rg -A 5 'SENTRY_AUTH_TOKEN' .github/workflows/
Length of output: 2062
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12462333581. |
|
Deploy-Preview-URL: https://ce-38301.dp.appsmith.com |
Failed server tests
|
Failed server tests
|
Failed server tests
|
|
Deploy-Preview-URL: https://ce-38301.dp.appsmith.com |
| import type { LINTER_TYPE } from "./constants"; | ||
| import type { Attributes } from "@opentelemetry/api"; |
There was a problem hiding this comment.
One theoretical concern with importing opentelemetry api directly across the codebase is that it introduces tight coupling between our code and the instrumentation library. It prevents us to switch between open telemetry and zipkin for example by just changing the library import and needs replacement everywhere in the code. What do you think ?
There was a problem hiding this comment.
Understood, we can move this to a common type in instrumentation/types.ts
There was a problem hiding this comment.
@rajatagrawal Resolved
dvj1988
force-pushed
the
chore/add-faro-integration
branch
from
db9d859 to
265ac2e
Compare
| @@ -35,7 +35,6 @@ | |||
| "postcss-url", | |||
| "@types/dom-view-transitions", | |||
| "@types/jest", | |||
| "@sentry/webpack-plugin", | |||
There was a problem hiding this comment.
"@sentry/react": "^6.2.4" — should this package also be deleted?
There was a problem hiding this comment.
We are planning to do this in the next pr.
app/client/public/index.html
Outdated
| otlpServiceName: parseConfig('{{env "APPSMITH_NEW_RELIC_OTEL_SERVICE_NAME"}}'), | ||
| otlpEndpoint:parseConfig('{{env "APPSMITH_NEW_RELIC_OTEL_EXPORTER_OTLP_ENDPOINT"}}'), | ||
|
|
||
| serviceInstanceId: parseConfig('{{env "APPSMITH_HOSTNAME"}}') || "appsmith-0", |
There was a problem hiding this comment.
Do the default values need to be defined here or can they be defined in the function that uses this environment variable to give final value for this property ?
There was a problem hiding this comment.
We are doing it in both the places now. I think we can remove it from here.
| @@ -371,7 +374,7 @@ function withWidgetProps(WrappedWidget: typeof BaseWidget) { | |||
| } | |||
| } | |||
|
|
|||
| if (!newRelic.enableNewRelic) { | |||
| if (!tracingUrl) { | |||
There was a problem hiding this comment.
nit: Is there merit in wrapping this logic into a fn : tracingEnabled() to improve readability ?
There was a problem hiding this comment.
yeah makes sense. Will add this.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/client/src/instrumentation/utils.ts (3)
15-37: Consider adding TypeScript types for better type safety.The memoized function implementation is clean and efficient. Consider adding explicit return type and parameter types for better type safety.
-const getAppParams = memoizeOne( +const getAppParams = memoizeOne<(origin: string, pathname: string, search: string) => { + appMode: string; + pageId?: string; + branchName?: string; + applicationSlug?: string; +}>(
39-53: Add SSR safety check for window object.The function assumes window object is always available. Consider adding a safety check for server-side rendering environments.
export const getCommonTelemetryAttributes = () => { + if (typeof window === 'undefined') { + return {}; + } const { origin, pathname, search } = window.location;
55-63: Simplify boolean return.The function can be simplified to directly return the boolean condition.
export const isTracingEnabled = () => { const { observability } = getAppsmithConfigs(); - if (observability.tracingUrl) { - return true; - } - return false; + return !!observability.tracingUrl; };app/client/src/instrumentation/index.ts (1)
32-54: Consider expanding ignoreUrls configuration.The ignoreUrls array only includes 'smartlook.cloud'. Consider maintaining this list in a configuration file for better maintainability and include other common analytics/monitoring services to prevent recursive tracing.
- const ignoreUrls = ["smartlook.cloud"]; + const ignoreUrls = [ + "smartlook.cloud", + "google-analytics.com", + "hotjar.com", + // Add more analytics services as needed + ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
app/client/package.json(2 hunks)app/client/public/index.html(1 hunks)app/client/src/index.tsx(3 hunks)app/client/src/instrumentation/index.ts(1 hunks)app/client/src/instrumentation/utils.ts(1 hunks)app/client/src/widgets/withWidgetProps.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/index.tsx
- app/client/src/widgets/withWidgetProps.tsx
- app/client/public/index.html
- app/client/package.json
🔇 Additional comments (3)
app/client/src/instrumentation/utils.ts (1)
1-13: LGTM! Dependencies and initialization look good.
The imports are well-organized and the session ID generation using nanoid is appropriate for telemetry purposes.
app/client/src/instrumentation/index.ts (2)
1-30: LGTM! Comprehensive setup for OpenTelemetry and Faro.
The imports and configuration setup are well-organized and include all necessary components for tracing.
78-86: LGTM! Good fallback handling.
The function properly handles cases where Faro is not initialized and provides appropriate fallback to default OpenTelemetry trace and context.
| const tracerProvider = new WebTracerProvider({ | ||
| resource: new Resource({ | ||
| [ATTR_DEPLOYMENT_NAME]: deploymentName, | ||
| [ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId, | ||
| [ATTR_SERVICE_NAME]: serviceName, | ||
| }), | ||
| }); | ||
|
|
||
| tracerProvider.addSpanProcessor( | ||
| new FaroSessionSpanProcessor( | ||
| new BatchSpanProcessor(new FaroTraceExporter({ ...faro })), | ||
| faro.metas, | ||
| ), | ||
| ); | ||
|
|
||
| tracerProvider.register({ | ||
| contextManager: new ZoneContextManager(), | ||
| }); | ||
|
|
||
| faro.api.initOTEL(trace, context); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for tracer initialization.
The tracer provider initialization should include error handling to prevent potential runtime issues.
+try {
tracerProvider.register({
contextManager: new ZoneContextManager(),
});
faro.api.initOTEL(trace, context);
+} catch (error) {
+ log.error("Failed to initialize tracer provider:", error);
+ // Fallback to default trace and context
+ faro = null;
+}Committable suggestion skipped: line range outside the PR's diff.
## Description - Remove new relic browser agent - Add faro sdk to capture frontend perf metrics and traces. 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/12490844984> > Commit: c9d4264 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12490844984&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 25 Dec 2024 09:33:26 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced new environment variable `APPSMITH_HOSTNAME` for dynamic hostname configuration in HTML files. - Enhanced telemetry capabilities with new imports and updated types for better observability. - Added `tracingUrl` under the observability section in configuration files for improved telemetry tracking. - **Bug Fixes** - Adjusted telemetry data handling to utilize new `Attributes` type for improved consistency. - **Documentation** - Updated import paths for various telemetry-related components to reflect new module organization. - **Chores** - Removed deprecated telemetry configurations and streamlined build processes. - Updated Nginx configuration to reflect new telemetry parameters. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
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/12490844984
Commit: c9d4264
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 25 Dec 2024 09:33:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
APPSMITH_HOSTNAMEfor dynamic hostname configuration in HTML files.tracingUrlunder the observability section in configuration files for improved telemetry tracking.Bug Fixes
Attributestype for improved consistency.Documentation
Chores