Skip to content

Conversation

@mydea
Copy link
Member

@mydea mydea commented Sep 11, 2023

Update: This PR was updated with new behaviour.

This PR updates the span reference cleanup to take into account if a span is/may still be referenced somewhere else.

Previously, whenever a span finished we removed the reference from the map, to clean up and avoid memory leaks.
However, it seems that sometimes spans are ended before a child span is started (at least the hooks may fire in this order). This leads to the potential case where a parent that should exist cannot be found, thus creating a new transaction instead of a span.

With this change, we keep more information in our span map, in order to clear sub-spans (=not transactions) only when the root span (=transaction) is finished.

@mydea mydea requested review from AbhiPrasad and lforst September 11, 2023 14:27
@mydea mydea self-assigned this Sep 11, 2023
@mydea mydea force-pushed the fn/node-otel-strict-mode branch from 3d53051 to 34985ce Compare September 11, 2023 15:17
@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.37 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.31 KB (0%)
@sentry/browser - Webpack (gzipped) 21.91 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 70.16 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.5 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.59 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 221.75 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 86.28 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 61.13 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.37 KB (+0.01% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.4 KB (0%)
@sentry/react - Webpack (gzipped) 21.94 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.27 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 50.88 KB (0%)

@mydea mydea force-pushed the fn/node-otel-strict-mode branch from 7b2826c to 1825c15 Compare September 12, 2023 09:51
@mydea mydea changed the title feat(node-otel): Implement new strict parent handling mode fix(node-otel): Refactor OTEL span reference cleanup Sep 12, 2023
@mydea mydea force-pushed the fn/node-otel-strict-mode branch 2 times, most recently from 452607a to 871e709 Compare September 12, 2023 11:35
@mydea mydea force-pushed the fn/node-otel-strict-mode branch from 871e709 to 7da7fc4 Compare September 12, 2023 11:36
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.

Really good change, makes sense to me. We might need to do similar in the other SDKs as well.

@mydea mydea merged commit 475c295 into develop Sep 12, 2023
@mydea mydea deleted the fn/node-otel-strict-mode branch September 12, 2023 14:11
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.

3 participants