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

[WIP] Use native OpenTelemetry Spans in node-experimental #9085

Closed
wants to merge 10 commits into from

Conversation

mydea
Copy link
Member

@mydea mydea commented Sep 21, 2023

This PR changes the performance handling of the node-experimental package fundamentally, aligning it even more with the OpenTelemetry model.

This is a WIP branch on top of some other small refactors that can/should be done to accommodate this - but to keep this as readable as possible, this PR should show only the most relevant changes for now.

Prerequisites

Preview Give feedback
  1. mydea
  2. mydea
  3. mydea

Tasks

Preview Give feedback

The core changes are:

  • startSpan / startInactiveSpan create Otel spans now, not Sentry Spans/Transactions
  • Generally, Sentry spans/transactions should be completely gone, from a users POV
  • Splits the functionality that was previously in the SpanProcessor into a new SpanProcessor + SpanExporter
  • Ditches the global map for state in favor of some WeakMaps, which means we do not need to clean our references up manually anymore etc.
  • Breadcrumbs are stored as events on the otel spans

How does transaction/span creation work now

In the old model, we would start transactions/spans in the span processor onStart and onEnd hook. This required us to keep track of the parent Sentry span, as we need it to call parentSpan.startSpan() in onStart. Since it can be tricky to know when a span is not needed anymore as a parent etc, this made garbage collection harder and messier, and also required us to still sprinkle Sentry spans/transactions everywhere through our code.

In the new model, only minimal processing is done in the span processor, and importantly, we do not create any Sentry spans yet. We store some additional data we need later in a WeakMap associated to the (Otel) span.

Then we leverage the underlying BatchSpanProcessor from OTEL, which collects spans together and sends them for processing to a SpanExporter. So only finished spans end up in our span processor. Our custom span exporter does the following:

  • Builds a tree hierarchy of the spans
  • Picks the root spans that are found, and builds transactions from there - adding children down the tree
  • Every span/transaction is immediately finished (with the correct end time) and then sent
  • Note that we store the current scope when the transaction was created and apply this scope.

For now I copied most of the stuff from opentelemetry-node over, eventually we can merge most of this together probably and export the parts from opentelemetry-node.

How do breadcrumbs work

We now pick all events added to spans and add them as breadcrumbs.
For this, we walk up the tree of spans up to the root and collect all breadcrumbs together. We use a special JSON field for now to actually store the breadcrumbs data (TODO: Maybe there is a better way to do this...).
When we add a breadcrumb, we actually always add it to the root span for now, not the active span. The reason is that this works better with our mental model of breadcrumbs, where anything that happens in this root span is relevant. But we'll also pick up any other events added by otel instrumentation along the way.

Open questions

  • Should we apply the scope when a span is started, or when it is finished? OTEL uses the context when it was started, and the model makes this easier to implement, but if we prefer we can probably also pick the current context/scope in onEnd. But not 100% sure how this would work with parallel spans, needs to be tested I guess.

@mydea mydea self-assigned this Sep 21, 2023
@mydea mydea force-pushed the fn/potel-native-spans branch 2 times, most recently from 0b205a2 to eba86fa Compare September 21, 2023 14:09
@github-actions
Copy link
Contributor

github-actions bot commented Sep 21, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 84.24 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.41 KB (-0.02% 🔽)
@sentry/browser - Webpack (gzipped) 22 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 78.69 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.52 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.59 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 254.14 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 86.42 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 61.23 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.38 KB (-0.02% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 84.26 KB (-0.02% 🔽)
@sentry/react - Webpack (gzipped) 22.03 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 102.23 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 50.98 KB (+0.06% 🔺)

@mydea mydea force-pushed the fn/potel-native-spans branch from eba86fa to a9ae41c Compare September 21, 2023 14:40
@mydea mydea force-pushed the fn/potel-wip-baseline branch from 946e66f to 193e67e Compare September 22, 2023 12:10
@mydea mydea force-pushed the fn/potel-native-spans branch 5 times, most recently from 52c7697 to 5ec4dab Compare September 25, 2023 15:00
@mydea mydea force-pushed the fn/potel-wip-baseline branch from 193e67e to a1cdd7c Compare September 25, 2023 15:02
@mydea mydea force-pushed the fn/potel-native-spans branch from 5ec4dab to 51c4ba6 Compare September 25, 2023 15:02
@mydea mydea force-pushed the fn/potel-wip-baseline branch from a1cdd7c to e8442a7 Compare September 26, 2023 08:16
@mydea mydea force-pushed the fn/potel-native-spans branch 2 times, most recently from 412b45c to 5f921b3 Compare September 26, 2023 13:20
@mydea mydea force-pushed the fn/potel-wip-baseline branch from e8442a7 to a7cc51e Compare September 27, 2023 13:14
@mydea mydea force-pushed the fn/potel-native-spans branch 2 times, most recently from 85b1659 to 71e2ebf Compare September 27, 2023 13:50
@mydea
Copy link
Member Author

mydea commented Sep 27, 2023

Note: This depends on two changes in existing packages which we'll need to merge first - see develop...fn/potel-wip-baseline:

  1. ref(core): Split transaction finish into underlying _finishTransaction: So we can extend the transaction and finish it with a custom scope
  2. feat(core): Extract sampleTransaction method out: So we can use our regular sampling in node-experimental

@mydea mydea force-pushed the fn/potel-native-spans branch from 71e2ebf to 1e3a929 Compare September 27, 2023 14:16
@mydea mydea force-pushed the fn/potel-native-spans branch 2 times, most recently from c7b929b to f50ba4c Compare September 29, 2023 13:26
mydea added a commit that referenced this pull request Oct 2, 2023
…tion` (#9137)

This makes it easier to extend the transaction (e.g. for POTEL) and make
use of the _finishing_ code without actually sending the transaction to
sentry.

This is needed for
#9085.
@mydea mydea force-pushed the fn/potel-wip-baseline branch from 75b0305 to 9fcee38 Compare October 2, 2023 11:09
@mydea mydea force-pushed the fn/potel-native-spans branch from f50ba4c to 021cb0d Compare October 2, 2023 11:09
@mydea mydea force-pushed the fn/potel-native-spans branch 3 times, most recently from 891c0ea to 3f53bf6 Compare October 2, 2023 13:39
mydea and others added 4 commits October 2, 2023 13:41
Bumps
[actions/upload-artifact](https://github.com/actions/upload-artifact)
from 3.1.2 to 3.1.3.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/actions/upload-artifact/releases">actions/upload-artifact's
releases</a>.</em></p>
<blockquote>
<h2>v3.1.3</h2>
<h2>What's Changed</h2>
<ul>
<li>chore(github): remove trailing whitespaces by <a
href="https://github.com/ljmf00"><code>@​ljmf00</code></a> in <a
href="https://redirect.github.com/actions/upload-artifact/pull/313">actions/upload-artifact#313</a></li>
<li>Bump <code>@​actions/artifact</code> version to v1.1.2 by <a
href="https://github.com/bethanyj28"><code>@​bethanyj28</code></a> in <a
href="https://redirect.github.com/actions/upload-artifact/pull/436">actions/upload-artifact#436</a></li>
</ul>
<p><strong>Full Changelog</strong>: <a
href="https://github.com/actions/upload-artifact/compare/v3...v3.1.3">https://github.com/actions/upload-artifact/compare/v3...v3.1.3</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/actions/upload-artifact/commit/a8a3f3ad30e3422c9c7b888a15615d19a852ae32"><code>a8a3f3a</code></a>
Merge pull request <a
href="https://redirect.github.com/actions/upload-artifact/issues/436">#436</a>
from bethanyj28/main</li>
<li><a
href="https://github.com/actions/upload-artifact/commit/7b48769c030f7121ecb01c3558dd3cd8b9660a20"><code>7b48769</code></a>
update dependency cache</li>
<li><a
href="https://github.com/actions/upload-artifact/commit/66630398dfe3d04deb4c489ac54b9b468f071706"><code>6663039</code></a>
update dist/index.js</li>
<li><a
href="https://github.com/actions/upload-artifact/commit/55e76b779da56f582e27a6f7aff54c1e610551e5"><code>55e76b7</code></a>
bump <code>@​actions/artifact</code> version</li>
<li><a
href="https://github.com/actions/upload-artifact/commit/65d862660abb392b8c4a3d1195a2108db131dd05"><code>65d8626</code></a>
chore(github): remove trailing whitespaces (<a
href="https://redirect.github.com/actions/upload-artifact/issues/313">#313</a>)</li>
<li>See full diff in <a
href="https://github.com/actions/upload-artifact/compare/v3.1.2...v3.1.3">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=actions/upload-artifact&package-manager=github_actions&previous-version=3.1.2&new-version=3.1.3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
[Gitflow] Merge master into develop
@mydea mydea force-pushed the fn/potel-native-spans branch from 3f53bf6 to f6fa256 Compare October 2, 2023 15:07
mydea added 2 commits October 3, 2023 09:22
This extracts the `sampleTransaction` method out and exports this from
`core` so we can use it for POTEL (where we build transactions
differently).
@mydea mydea force-pushed the fn/potel-native-spans branch from f6fa256 to 1531c77 Compare October 3, 2023 07:37
@mydea mydea deleted the branch fn/potel-wip-baseline October 3, 2023 07:37
@mydea mydea closed this Oct 3, 2023
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.

5 participants