@W-18760780 Integrate opentelemetry into performance.js by jeremy-jung1 · Pull Request #2722 · SalesforceCommerceCloud/pwa-kit

jeremy-jung1 · 2025-07-02T22:58:51Z

Integrating opentelemetry spans into performance.js

Description

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

(change1) Added opentelemetry span usage and some cleanups in react-rendering to prevent memory leak
(change2) Modified the OTEL config so that it doesn't throw in the browser

How to Test-Drive This PR

(step1) pull this branch locally
(step2) go to pwa-kit-react-sdk and do npm run build
(step3) then go to template-retail-react-app and do export OTEL_SDK_ENABLED=true && npm start
(step3) Go to http://localhost:3000/?__server_timing. See that you can see logs of spans:

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

…ncejs

cc-prodsec · 2025-07-02T22:59:01Z

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

…ncejs

packages/pwa-kit-react-sdk/src/utils/opentelemetry.js

hajinsuha1 · 2025-07-11T18:51:22Z

packages/pwa-kit-react-sdk/src/utils/opentelemetry-config.test.js

+            delete process.env.OTEL_SERVICE_NAME
+            delete process.env.OTEL_SDK_ENABLED
+            delete process.env.OTEL_B3_TRACING_ENABLED
+
+            const {getOTELConfig} = mockModule()
+            const config = getOTELConfig()
+
+            expect(config).toEqual({
+                serviceName: 'pwa-kit-react-sdk',
+                enabled: false,
+                b3TracingEnabled: false
+            })
+        })


nit: insead of testing the combination of all the env variables would we consider having separate tests for each config?

should return service name when OTEL_SERVICE_NAME is set

should return default service when OTEL_SERVICE_NAME is not set

should return enabled when OTEL_SDK_ENABLED is set

should return disabled when OTEL_SDK_ENABLED is not set
...

That way if additional configs are added in the future each property is isolated

hajinsuha1

one nit comment but followed the test steps and can see the open telemetry logs

adamraya · 2025-07-16T01:50:58Z

packages/pwa-kit-react-sdk/src/utils/performance.js

+                        }, this.maxSpanDuration)
+                        this.spanTimeouts.set(name, timeoutId)
+                    }
                }


Should we log a warning in case we attempt to create a duplicate span name?

adamraya · 2025-07-16T01:53:19Z

packages/pwa-kit-react-sdk/src/utils/performance.js

+                        endMark,
+                        namespace: 'PerformanceTimer.mark'
+                    })
                }


Should we also call cleanup in case of catching an error?

The timed out automatic cleanup made for each span on creation would clean them up eventually. Even when endSpan logs an error within itself.

adamraya · 2025-07-16T01:58:40Z

packages/pwa-kit-react-sdk/src/utils/performance.js

    mark(name, type, options = {}) {
-        if (!this.enabled) {
+        const {detail = ''} = options
+        if (!name || !type || !this.enabled) {


Should we keep checking if enabled first, then validate inputs?

Suggested change

if (!name || !type || !this.enabled) {

if (!this.enabled || !name || !type) {

adamraya · 2025-07-16T02:31:19Z

packages/pwa-kit-react-sdk/src/ssr/server/react-rendering.js

Following the steps to test the changes I see in the logs we have a few "No parent span found" warnings and it looks like each span is using a different traceId instead of all spans sharing the same traceId.

... pwa-kit-react-sdk.opentelemetry.logSpanData INFO OpenTelemetry span data {"traceId":"d8acdf23f3344d04d9da11e4dc6a6fed","name":"ssr.render-to-string","id":"e0a43b64a9e330bd","kind":0,"timestamp":"2025-07-16T02:09:20.417000000Z","duration":0,"attributes":{"service.name":"pwa-kit-react-sdk","performance_type":"start","performance.mark":"ssr.render-to-string","performance.type":"start","performance.detail":"","event":"start"},"status":{"code":0},"events":[],"links":[],"start_time":[1752631760,417000000],"end_time":[1752631760,417000000],"forwardTrace":false} pwa-kit-react-sdk.opentelemetry.logSpanData INFO OpenTelemetry span data {"traceId":"d8acdf23f3344d04d9da11e4dc6a6fed","name":"ssr.render-to-string","id":"e0a43b64a9e330bd","kind":0,"timestamp":"2025-07-16T02:09:20.417000000Z","duration":68.30075,"attributes":{"service.name":"pwa-kit-react-sdk","performance_type":"start","performance.mark":"ssr.render-to-string","performance.type":"start","performance.detail":"","event":"end"},"status":{"code":1},"events":[],"links":[],"start_time":[1752631760,417000000],"end_time":[1752631760,485300750],"forwardTrace":false} pwa-kit-react-sdk.opentelemetry.logSpanData INFO OpenTelemetry span data {"traceId":"714bd3edf17e826c55ed0dabb8f2cc38","name":"ssr.total","id":"31e2c7b3b61ff8f1","kind":0,"timestamp":"2025-07-16T02:09:19.220000000Z","duration":1266.033834,"attributes":{"service.name":"pwa-kit-react-sdk","performance_type":"start","performance.mark":"ssr.total","performance.type":"start","performance.detail":"","event":"end"},"status":{"code":1},"events":[],"links":[],"start_time":[1752631759,220000000],"end_time":[1752631760,486033834],"forwardTrace":false} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.route-matching"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.load-component"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.fetch-strategies.react-query.pre-render"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.fetch-strategies.react-query.use-query.1"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.fetch-strategies.react-query.use-query.useCategory-0"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.fetch-strategies.react-query.use-query.useProductSearch-2"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.fetch-strategies"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.render-to-string"} pwa-kit-react-sdk.opentelemetry WARN No parent span found in context {"metricName":"ssr.total"}

Are we missing wrapping the render function with a root span? Is this something we plan to do in a separated ticket?
https://github.com/SalesforceCommerceCloud/pwa-kit/blob/W-18129479-spike-o11y/packages/pwa-kit-react-sdk/src/ssr/server/react-rendering.js#L136

Yeah that will be done in this ticket: https://gus.lightning.force.com/lightning/_classic/%2Fa07EE00002G04HYYAZ

jeremy-jung1 added 3 commits July 1, 2025 19:33

initial file

20eb9b4

Uncomment global.performance

7b349b6

Merge branch 'W-18759774-opentelemetry-util' into W-18760780-performa…

07b1206

…ncejs

jeremy-jung1 requested a review from a team as a code owner July 2, 2025 22:58

jeremy-jung1 added 15 commits July 7, 2025 16:35

Some comments and namespacing

3326746

linting

051ea73

Some dev only flags and mods

f801b5b

more info

ce666ec

More dev

ef27e9c

Update performance.test.js

02cbb52

remove some console logs

bb36629

Updates

1e905cf

Update performance.test.js

0dd809d

Dev mod

1e586d9

Add tests and more functionality

48ff1b9

Merge branch 'W-18759774-opentelemetry-util' into W-18760780-performa…

299039c

…ncejs

Fix tests and add cleanup

dfb8812

linting

20b8c96

fix tests

1d9a905

jeremy-jung1 changed the title ~~@W-18760780 performance.js~~ @W-18760780 Integrate opentelemetry into performance.js Jul 10, 2025

jeremy-jung1 added 2 commits July 10, 2025 15:00

Merge branch 'W-18759774-opentelemetry-util' into W-18760780-performa…

fc1f8f5

…ncejs

remove dev only changes and add more tests

7171b8f

jeremy-jung1 commented Jul 10, 2025

View reviewed changes

packages/pwa-kit-react-sdk/src/utils/opentelemetry.js Show resolved Hide resolved

jeremy-jung1 changed the base branch from feature/opentelemetry to W-18759774-opentelemetry-util July 10, 2025 22:18

jeremy-jung1 added 5 commits July 10, 2025 15:31

Remove handling of detail being an object

943a40c

Some temporary? changes to get it to work

4072966

Update config

6c5430d

bundlesize increase

24b37e0

Update CHANGELOG.md

6c301c5

Update react-rendering.js

5a0f754

hajinsuha1 reviewed Jul 11, 2025

View reviewed changes

hajinsuha1 approved these changes Jul 11, 2025

View reviewed changes

jeremy-jung1 changed the base branch from W-18759774-opentelemetry-util to feature/opentelemetry July 14, 2025 06:12

Apply feedback

e8dc896

adamraya reviewed Jul 16, 2025

View reviewed changes

adamraya approved these changes Jul 16, 2025

View reviewed changes

Apply feedback

894549b

jeremy-jung1 merged commit 0d75e65 into feature/opentelemetry Jul 16, 2025
35 checks passed

jeremy-jung1 mentioned this pull request Aug 20, 2025

@W-18900367 Integration of opentelemetry for SSR Tracing #3133

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-18760780 Integrate opentelemetry into performance.js#2722

@W-18760780 Integrate opentelemetry into performance.js#2722
jeremy-jung1 merged 28 commits intofeature/opentelemetryfrom
W-18760780-performancejs

jeremy-jung1 commented Jul 2, 2025 •

edited

Loading

Uh oh!

cc-prodsec commented Jul 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

hajinsuha1 Jul 11, 2025

Uh oh!

hajinsuha1 left a comment

Uh oh!

adamraya Jul 16, 2025

Uh oh!

adamraya Jul 16, 2025

Uh oh!

jeremy-jung1 Jul 16, 2025

Uh oh!

adamraya Jul 16, 2025

Uh oh!

adamraya Jul 16, 2025

Uh oh!

jeremy-jung1 Jul 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (!name \|\| !type \|\| !this.enabled) {
	if (!this.enabled \|\| !name \|\| !type) {

Conversation

jeremy-jung1 commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

Uh oh!

cc-prodsec commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

Uh oh!

hajinsuha1 Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

hajinsuha1 left a comment

Choose a reason for hiding this comment

Uh oh!

adamraya Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

adamraya Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

jeremy-jung1 Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

adamraya Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

adamraya Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

jeremy-jung1 Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jeremy-jung1 commented Jul 2, 2025 •

edited

Loading

cc-prodsec commented Jul 2, 2025 •

edited

Loading