Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix broken serialization of node transaction spans #419

Merged
merged 3 commits into from
Feb 23, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Feb 23, 2022

Fixes #418

The Electron SDK has a mergeEvents function which uses deepmerge. This saves having a large manual merge function which has to be kept up to date if any of the event objects change.

The Sentry tracing Span has a custom toJSON implementation to convert camelCase properties to snake_case to match the Sentry API guidelines. When the event gets merged with the defaults this function is lost.

This PR:

  • Manually copies the spans so their toJSON function is still there
  • Adds an e2e test to ensure this doesn't break in the future
  • Improves the e2e event normalisation code so it doesn't add properties

@timfish timfish changed the title fit: Copy spans when merging events fit: Fix broken serialization of node transaction spans Feb 23, 2022
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Do we need to do similar for sessions as well? I assume it's not a problem for now because sessions don't got through the event processors or from renderer -> main process.

test/e2e/test-apps/other/custom-tracing/src/main.js Outdated Show resolved Hide resolved
test/e2e/test-apps/other/custom-tracing/src/main.js Outdated Show resolved Hide resolved
@timfish timfish changed the title fit: Fix broken serialization of node transaction spans fix: Fix broken serialization of node transaction spans Feb 23, 2022
@timfish
Copy link
Collaborator Author

timfish commented Feb 23, 2022

Do we need to do similar for sessions as well?

I think not. Sessions don't go through the event processors and we don't touch them otherwise.

@timfish timfish merged commit fb04679 into getsentry:master Feb 23, 2022
@timfish timfish deleted the fix/node-transactions branch March 3, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Transactions are not showing in the Performance tab
2 participants