chore(frontend): agent configuration#95
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements frontend agent configuration updates for RUM (Real User Monitoring) instrumentation and testing. The changes update the Splunk RUM agent to use a "next" version, add unique IDs to UI elements for better tracking, and introduce an intentionally non-functional "Show More Reviews" button to test rage click detection.
Changes:
- Updated RUM agent configuration to use CDN path
/next/and added frustration signals tracking with rage click detection - Added unique
idattributes to multiple UI buttons and form elements across the frontend - Changed Docker image references from official Splunk registry to joozty/test1 for testing purposes
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frontend/pages/_document.tsx | Updated RUM agent initialization scripts and configuration |
| src/frontend/components/Button/Button.tsx | Made id prop required in Button interface |
| src/frontend/components/ProductReviews/* | Added non-functional "Show More Reviews" button for rage click testing |
| src/frontend/components/* | Added id attributes to various buttons and UI elements |
| src/frontend/pages/* | Added id attributes to page-level buttons |
| src/*/*-k8s.yaml | Changed image references to joozty/test1 for testing |
| kubernetes/TEST-splunk-astronomy-shop-test1.yaml | Large generated consolidated K8s manifest with test images |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/frontend/pages/_document.tsx
Outdated
| }} | ||
| /> | ||
| <Script | ||
| src={`https://cdn.signalfx.com/o11y-gdi-rum/next/splunk-otel-web-session-recorder.js`} |
There was a problem hiding this comment.
The CDN URL references a "next" version path (https://cdn.signalfx.com/o11y-gdi-rum/next/splunk-otel-web-session-recorder.js). Using "next" as a version identifier is risky because: 1) It may point to unstable or pre-release code that could break without notice, 2) The behavior may change unexpectedly as new versions are deployed, 3) It's unclear if "next" is a valid version path on this CDN. Consider using a specific stable version number instead, such as "latest" or a pinned version like "2.0.5" (which is referenced in the main RUM configuration). This ensures predictable behavior and prevents unexpected breaking changes in production.
There was a problem hiding this comment.
it points always to the latest commit in the agent repo and it is intentionall
| dangerouslySetInnerHTML={{ __html: this.props.envString }}> | ||
| </Script> | ||
| {/* Load user attributes generator - must load before RUM initialization */} | ||
| <script src="/global-attributes.js"></script> |
There was a problem hiding this comment.
This script tag uses the older HTML script element instead of Next.js's Script component, while other scripts in this file have been migrated to use the Script component. For consistency and to benefit from Next.js's script optimization features, consider updating this to use the Script component with strategy="beforeInteractive" like the other scripts.
| privacy: { | ||
| "maskAllText": false | ||
| }, |
There was a problem hiding this comment.
The previous privacy configuration included sensitivityRules that allowed for granular control over what content gets masked or excluded from tracking (e.g., masking all p tags by default, unmasking specific elements, excluding user profile sections). These rules have been completely removed in favor of a simple "maskAllText: false" setting, which means all text will now be captured without any filtering. This is a significant reduction in privacy controls. Consider restoring or reimplementing appropriate sensitivity rules to protect user privacy while still allowing necessary monitoring functionality.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _experimental_dataAttributesToCapture: [ | ||
| "data-cy", | ||
| ], | ||
| _experimental_spaMetrics: true |
There was a problem hiding this comment.
The configuration uses experimental features (_experimental_dataAttributesToCapture and _experimental_spaMetrics) which may change or be removed in future versions. These features are prefixed with _experimental_ indicating they are not stable APIs.
Consider documenting the dependency on these experimental features and monitoring for changes in future RUM versions, or verify if these features have been stabilized in the "next" version being used.
| privacy: { | ||
| "maskAllText": false | ||
| }, | ||
| instrumentations: { | ||
| "frustrationSignals": { | ||
| "rageClick": true | ||
| } | ||
| }, | ||
| _experimental_dataAttributesToCapture: [ | ||
| "data-cy", | ||
| ], | ||
| _experimental_spaMetrics: true | ||
| }); | ||
|
|
||
| // Initialize tracer for custom spans and expose globally | ||
| const Provider = SplunkRum.provider; | ||
| var tracer = Provider.getTracer('appModuleLoader'); | ||
| const tracer = SplunkRum.provider.getTracer('appModuleLoader'); | ||
| window.tracer = tracer; // Make tracer available globally for custom spans | ||
|
|
||
| if (typeof SplunkSessionRecorder !== 'undefined') { | ||
| `, | ||
| }} | ||
| /> | ||
| <script | ||
| src="https://cdn.signalfx.com/o11y-gdi-rum/next/splunk-otel-web-session-recorder.js" | ||
| crossOrigin="anonymous" | ||
| /> | ||
| <script | ||
| id="splunk-session-recorder-init" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: ` | ||
| SplunkSessionRecorder.init({ | ||
| realm: realm, | ||
| rumAccessToken: rumAccessToken | ||
| realm: window.ENV.SPLUNK_RUM_REALM, | ||
| rumAccessToken: window.ENV.SPLUNK_RUM_TOKEN, | ||
| maskAllText: false |
There was a problem hiding this comment.
The configuration contains two different privacy/masking settings that may be conflicting:
- In SplunkRum.init(),
privacy.maskAllTextis set tofalse(line 88) - In SplunkSessionRecorder.init(),
maskAllTextis set tofalse(line 118)
While both are currently set to false, this duplication could lead to confusion. The previous version had more granular privacy controls with sensitivityRules. Consider documenting why the privacy rules were simplified, as this changes the data collection behavior significantly.
| onClick={() => { | ||
| // Intentionally empty - no action for rage click testing | ||
| }} | ||
| data-cy="ShowAllReviews" |
There was a problem hiding this comment.
The button is intentionally non-functional but doesn't communicate this to assistive technology users. Consider adding an aria-label or aria-describedby attribute that indicates this is a test button, or disable the button with the disabled attribute and appropriate styling if the intent is to test clicking on disabled elements.
This would improve accessibility while still serving the testing purpose.
| data-cy="ShowAllReviews" | |
| data-cy="ShowAllReviews" | |
| aria-label="Show all reviews (test button that does not perform any action)" |
|
|
||
| const Button = styled.button<{ $type?: 'primary' | 'secondary' | 'link' }>` | ||
| interface ButtonProps { | ||
| id: string; |
There was a problem hiding this comment.
The id prop is marked as required in the ButtonProps interface. This creates a breaking change because:
- Styled components that extend Button (e.g.,
styled(Button)in CartButton, EmptyCartButton, GoShoppingButton, AddToCart) will now require theidprop to be passed by consumers - The
idattribute is a standard HTML attribute that should be optional
The id prop should be marked as optional by changing id: string; to id?: string; to maintain backward compatibility and follow HTML attribute conventions.
| id: string; | |
| id?: string; |
| <script | ||
| src="https://cdn.signalfx.com/o11y-gdi-rum/next/splunk-otel-web-session-recorder.js" | ||
| crossOrigin="anonymous" |
There was a problem hiding this comment.
Similar to the main RUM script, the session recorder is also using the "next" version tag which points to pre-release versions. For production stability, consider using "latest" or a specific version number.
| <script | ||
| id="splunk-rum-init" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: ` | ||
| SplunkRum.init({ | ||
| realm: realm, | ||
| rumAccessToken: rumAccessToken, | ||
| applicationName: applicationName, | ||
| deploymentEnvironment: deploymentEnvironment, | ||
| realm: window.ENV.SPLUNK_RUM_REALM, | ||
| rumAccessToken: window.ENV.SPLUNK_RUM_TOKEN, | ||
| applicationName: window.ENV.SPLUNK_APP_NAME, | ||
| deploymentEnvironment: window.ENV.SPLUNK_ENV, | ||
| globalAttributes: getSplunkGlobalAttributes(), | ||
| version: '2.0.5', | ||
| globalAttributes: globalAttributes, | ||
| environment: '', | ||
| // Digital Experience Analytics configuration | ||
| user: { | ||
| trackingMode: 'anonymousTracking' | ||
| }, | ||
| // Privacy controls | ||
| maskAllText: true, | ||
| sensitivityRules: [ | ||
| { rule: 'mask', selector: 'p' }, | ||
| { rule: 'unmask', selector: 'p.item-label' }, | ||
| { rule: 'exclude', selector: '#user-profile' } | ||
| ] | ||
| privacy: { | ||
| "maskAllText": false | ||
| }, | ||
| instrumentations: { | ||
| "frustrationSignals": { | ||
| "rageClick": true | ||
| } | ||
| }, | ||
| _experimental_dataAttributesToCapture: [ | ||
| "data-cy", | ||
| ], | ||
| _experimental_spaMetrics: true | ||
| }); | ||
|
|
||
| // Initialize tracer for custom spans and expose globally | ||
| const Provider = SplunkRum.provider; | ||
| var tracer = Provider.getTracer('appModuleLoader'); | ||
| const tracer = SplunkRum.provider.getTracer('appModuleLoader'); | ||
| window.tracer = tracer; // Make tracer available globally for custom spans | ||
|
|
||
| if (typeof SplunkSessionRecorder !== 'undefined') { | ||
| `, |
There was a problem hiding this comment.
The previous implementation had a retry mechanism (setTimeout(initializeSplunkRUM, 100)) that would retry initialization if SplunkRum was not yet defined. The new implementation removes this retry logic.
With the new approach, if the Splunk RUM script hasn't fully loaded when the initialization script runs, the initialization will fail silently. While the script loading order should prevent this (load script, then run init), network conditions or browser behavior could cause race conditions.
Consider adding error handling or verification that SplunkRum is defined before calling init(), or document why this retry logic is no longer needed with the new CDN version.
| <script | ||
| id="splunk-session-recorder-init" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: ` | ||
| SplunkSessionRecorder.init({ | ||
| realm: realm, | ||
| rumAccessToken: rumAccessToken | ||
| realm: window.ENV.SPLUNK_RUM_REALM, | ||
| rumAccessToken: window.ENV.SPLUNK_RUM_TOKEN, | ||
| maskAllText: false | ||
| }); | ||
| } | ||
| } else { | ||
| setTimeout(initializeSplunkRUM, 100); | ||
| } | ||
| })(); | ||
| ` | ||
| }} /> | ||
| `, | ||
| }} |
There was a problem hiding this comment.
Similar to the RUM initialization, the session recorder initialization no longer has the safety check to verify that SplunkSessionRecorder is defined before calling init().
Consider adding error handling or verification that SplunkSessionRecorder is defined, or document why this is safe with the new implementation.
Changes
Few minor updates in frontend package