Skip to content

fix: avoid overriding route span attributes in PageSpanProcessor#968

Merged
joaquin-diaz merged 5 commits into
mainfrom
joaquin-diaz/fix/EMBR-9614-fix-page-span-processor
Nov 14, 2025
Merged

fix: avoid overriding route span attributes in PageSpanProcessor#968
joaquin-diaz merged 5 commits into
mainfrom
joaquin-diaz/fix/EMBR-9614-fix-page-span-processor

Conversation

@joaquin-diaz
Copy link
Copy Markdown
Contributor

Goal

  • avoid overriding route span attributes in PageSpanProcessor by adding
if (span.attributes[KEY_EMB_TYPE] === EMB_TYPES.Surface) {
  // Don't override page attributes for surface spans
  return;
}

Testing

We didn't caught this because all our tests were very isolated and none of them was using the PageSpanProcessor combined with the instrumentation tests. React had nothing to do with it but just to have more coverage for the future I added unit tests that render a react app with a router set up to test this more broadly

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 12, 2025

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 1 package(s) with unknown licenses.
See the Details below.

License Issues

package-lock.json

PackageVersionLicenseIssue Type
react-router-domv6plus7.9.6NullUnknown License

OpenSSF Scorecard

PackageVersionScoreDetails
npm/react-router 7.9.6 🟢 4.6
Details
CheckScoreReason
Code-Review⚠️ 1Found 3/27 approved changesets -- score normalized to 1
Maintained🟢 1030 commit(s) and 7 issue activity found in the last 90 days -- score normalized to 10
Security-Policy🟢 10security policy file detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Packaging⚠️ -1packaging workflow not detected
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
Binary-Artifacts🟢 10no binaries found in the repo
License🟢 10license file detected
Pinned-Dependencies⚠️ 0dependency not pinned by hash detected -- score normalized to 0
Signed-Releases⚠️ -1no releases found
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: some github tokens can't read classic branch protection rules: https://github.com/ossf/scorecard-action/blob/main/docs/authentication/fine-grained-auth-token.md
Fuzzing⚠️ 0project is not fuzzed
SAST⚠️ 0SAST tool is not run on all commits -- score normalized to 0
Vulnerabilities⚠️ 046 existing vulnerabilities detected
npm/react-router-domv6plus 7.9.6 UnknownUnknown
npm/react-router-domv4v5 npm:react-router-dom@5.3.4 UnknownUnknown
npm/react-router-domv6plus npm:react-router-dom@7.9.6 UnknownUnknown

Scanned Files

  • package-lock.json
  • package.json

Comment thread web-test-runner.config.js
files: ['src/**/*.test.ts', 'src/**/*.test.tsx'],
plugins: [
vitePlugin({
optimizeDeps: {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@overbalance I had to remove this because when running react inside the tests it was trying to import everything from ESM, I tried to add all the exceptions here but the list just started to grow with a lot of internal libraries and I didn't see the point of keep adding them. Let me know if there's anything else we can do here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added specifically to deal with test failures in CI: #709, which do seem to have reoccurred here with the removal: https://github.com/embrace-io/embrace-web-sdk/actions/runs/19309537240/job/55225832277?pr=968

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked with Jared to get this working, we think in the future we can move to vitest browser directly to avoid using web-test-runner with these weird configurations

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 12, 2025

Performance results

CDP Performance Tests

Number of Requests Size of Requests Script Duration Task Duration Heap Used Size
Requests +3 requests +29.73 KB
Page Loaded +15.49 ms +6.29 ms +0.91 MB
Generate 100 fetch requests +16.69 ms +72.48 ms +1.97 MB
Generate 100 XHR requests +38.97 ms +113.08 ms +2.30 MB
Click 100 buttons and generate 100 logs +22.00 ms +26.87 ms +2.85 MB
Throw a 100 exceptions +2.52 ms +17.08 ms +2.08 MB
End Session +7.60 ms +15.03 ms +3.01 MB
Total +3 requests +29.73 KB +103.27 ms +250.82 ms +13.12 MB

Lighthouse Startup Performance Tests

Difference Description
Total Blocking Time 0 ms Difference in Total Blocking Time: Sum of all time periods between FCP and Time to Interactive, when task length exceeded 50ms, expressed in milliseconds. Learn more about the Total Blocking Time metric.
Main Thread Time +54.37 ms Difference in Main Thread Time: Consider reducing the time spent parsing, compiling and executing JS. You may find delivering smaller JS payloads helps with this. Learn how to minimize main-thread work
Script Evaluation Time +43.28 ms Difference in Script Evaluation Time: Consider reducing the time spent parsing, compiling, and executing JS. You may find delivering smaller JS payloads helps with this. Learn how to reduce Javascript execution time.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 12, 2025

build results

vite-7 Platform Tests

Total Uncompressed Size Total Gzip Size
vite-7 - esnext +165.25 KB +47.82 KB
vite-7 - es2015 +172.12 KB +49.50 KB

vite-otel-latest Platform Tests

Total Uncompressed Size Total Gzip Size
vite-otel-latest - esnext +164.33 KB +49.52 KB
vite-otel-latest - es2015 +171.12 KB +51.15 KB

webpack-5 Platform Tests

Total Uncompressed Size Total Gzip Size
webpack-5 - esnext +122.81 KB +43.23 KB
webpack-5 - es2015 +123.49 KB +44.69 KB

Comment thread web-test-runner.config.js
files: ['src/**/*.test.ts', 'src/**/*.test.tsx'],
plugins: [
vitePlugin({
optimizeDeps: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added specifically to deal with test failures in CI: #709, which do seem to have reoccurred here with the removal: https://github.com/embrace-io/embrace-web-sdk/actions/runs/19309537240/job/55225832277?pr=968

Comment thread src/processors/PageSpanProcessor/PageSpanProcessor.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these modelled after waitFor/render from '@testing-library/react'? Should we just include that as another dev dependency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did them manually but yes I would guess it's pretty much the same. I didn't want to add an extra dependency for 2 simple functions, maybe in the future if we see the need of keep testing react deeper?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep sounds good

Copy link
Copy Markdown
Member

@overbalance overbalance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend pinning the react-router-dom deps to newest version in order to match the existing package.json convention.

@joaquin-diaz joaquin-diaz merged commit 39433a6 into main Nov 14, 2025
17 checks passed
@joaquin-diaz joaquin-diaz deleted the joaquin-diaz/fix/EMBR-9614-fix-page-span-processor branch November 14, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants