feat: Add scope.getTransaction, rename scope.setTransaction -> setTransactionName by HazAT · Pull Request #2668 · getsentry/sentry-javascript

HazAT · 2020-06-11T09:49:36Z

Motivation

We want to have an easy way for users to retrieve a Transaction that is set on the Scope.
Right now we only have an API for retrieving the Span:

const span = Sentry.getCurrentHub().getScope().getSpan();

This introduces scope.getTransaction.

const transaction = Sentry.getCurrentHub().getScope().getTransaction();

which could return undefined if there is no Transaction going.

Also (in the title)
rename scope.setTransaction -> setTransactionName to make it clear that getTransaction doesn't return a string.

getsentry-bot · 2020-06-11T09:56:27Z

	Messages
📖	@sentry/browser bundle gzip'ed minified size: (ES5: 17.1387 kB) (ES6: 16.2529 kB)
📖	✅ TSLint passed

Generated by 🚫 dangerJS against fd23147

packages/apm/src/hubextensions.ts

rhcarvalho

I feel getSpan that does not return a Span is an awkward API addition.

For a parallel, see getScope(): Scope | undefined and withScope(callback: (scope: Scope) => void): void

packages/apm/src/span.ts

packages/minimal/src/index.ts

packages/apm/src/span.ts

…vascript into hazat/getTransaction

CHANGELOG.md

packages/hub/src/scope.ts

rhcarvalho

Please let's not do getXXX that does not return XXX. This is confusing naming right in the face of API users. We already have withXXX for APIs that take a callback and return nothing.

This potential withTransaction on the Scope only solve the problem of setting the transaction name half-way.

Instead of Sentry.getCurrentHub().getScope().getSpan(), we still need to reach for the Scope with Sentry.getCurrentHub().withScope or ...configureScope which makes using it quite verbose. I noted we moved away from adding something to the static API as we had in a previous iteration of this PR?

I have a proposal:

Sentry.withTransaction((t) => {
    t.name = "...";
})

In fact, this pattern would be similar to doing context managers in Python -- Sentry.withTransaction could start a new transaction and finish it after the callback returns -- users will never forget to call finish and have access to change the name directly.

with Transaction(name="...") as t:
   child = t.startChild(...)
   # ...
   child.finish()

CHANGELOG.md

packages/apm/src/span.ts

packages/apm/test/hub.test.ts

rhcarvalho · 2020-06-16T11:23:59Z

packages/types/src/scope.ts

+  /**
+   * Returns the current set span if there is one
+   */
+  getSpan(): Span | undefined;


Do we still want getSpan or is this a left over?

We still want it, right now we don't have a case in JS where we need it but I think as soon as we start thinking about adding ORM (database) support to node we'll have the same need as python has right now.

That's interesting. Last time I looked at the Python SDK, it only had one slot to store a "span" in the scope and it is either a span or transaction.

If we want to have a slot for both and distinguish what getSpan and getTransaction do, we need to change the scope impl.

cc @untitaker

Co-authored-by: Abhijeet Prasad <[email protected]>

CHANGELOG.md

packages/apm/src/span.ts

packages/apm/test/hub.test.ts

untitaker · 2020-06-16T13:37:17Z

Sorry I wrote that review in the morning and forgot to submit

dcramer · 2020-06-16T15:14:03Z

@rhcarvalho the main need is to not create a new transaction, but rather capture the active transaction so that we can mutate it. Given that case the context manager style approach wouldn't be helpful here.

untitaker · 2020-06-16T15:14:50Z

@dcramer I think rodolfo wrote the same thing

rhcarvalho

PR description needs an update callback => getter.

The current motivation seems a bit off, since we don't really solve for shortening Sentry.getCurrentHub().getScope().getSpan() (it can already be shorter today with Sentry.withScope).

Seems that the major change being proposed here is making getSpan and getTransaction "blessed for public use" (getSpan was considered internal and is still documented as such).

packages/hub/src/scope.ts

packages/types/src/scope.ts

packages/types/src/span.ts

kamilogorek · 2020-06-22T08:38:59Z

packages/apm/test/hub.test.ts

    jest.useRealTimers();
  });

+  describe('getTransaction', () => {


These tests are not technically "correct". defined/undefined test should be moved to scope.test.ts, as this functionality is owned by the Scope, and tests below should only assert that a given method was called on the scope. expect(s.getTransaction).toBeCalled().

Moved to scope.test.ts

kamilogorek · 2020-06-22T08:41:11Z

packages/hub/src/scope.ts

   */
-  public setTransaction(transaction?: string): this {
-    this._transaction = transaction;
+  public setTransactionName(name?: string): this {


offtopic: This whole transaction thing will be very confused (and possibly broken) if someone uses Sentry.Integrations.Transaction in node env.

kamilogorek · 2020-06-22T08:42:00Z

packages/hub/src/scope.ts

+   * Can be removed in major version.
+   * @deprecated in favor of {@link this.setTransactionName}
+   */
+  public setTransaction(name?: string): this {


kilobytes, kilobytes 😶

kamilogorek · 2020-06-22T08:42:53Z

packages/hub/src/scope.ts

+   */
+  public getTransaction(): Transaction | undefined {
+    const span = this.getSpan() as Span & { spanRecorder: { spans: Span[] } };
+    if (span) {


This can be inline in one check, as there're no other if branches

kamilogorek

Approved code wise. I didn't take part in API design, so don't sue me please.

feat: Add Sentry.getTransaction

5728da3

HazAT requested a review from kamilogorek as a code owner June 11, 2020 09:49

HazAT self-assigned this Jun 11, 2020

fix: Linter

4cd4146

dcramer reviewed Jun 11, 2020

View reviewed changes

packages/apm/src/hubextensions.ts Outdated Show resolved Hide resolved

ref: Introduce Sentry.getSpan

a116d0f

HazAT changed the title ~~feat: Add Sentry.getTransaction~~ feat: Add Sentry.getSpan Jun 15, 2020

Merge branch 'master' into hazat/getTransaction

eeb80d0

rhcarvalho suggested changes Jun 15, 2020

View reviewed changes

packages/apm/src/span.ts Outdated Show resolved Hide resolved

packages/apm/src/span.ts Outdated Show resolved Hide resolved

packages/minimal/src/index.ts Outdated Show resolved Hide resolved

AbhiPrasad reviewed Jun 15, 2020

View reviewed changes

packages/apm/src/span.ts Outdated Show resolved Hide resolved

HazAT added 4 commits June 15, 2020 20:41

ref: Remove getSpan and use scope.getTransaction

9a9a360

Merge branch 'hazat/getTransaction' of github.com:getsentry/sentry-ja…

2b08c65

…vascript into hazat/getTransaction

fix: Linter

24e2494

ref: Rename setTransaction

0faac9b

HazAT changed the title ~~feat: Add Sentry.getSpan~~ feat: Add scope.getTransaction Jun 16, 2020

HazAT changed the title ~~feat: Add scope.getTransaction~~ feat: Add scope.getTransaction, rename scope.setTransaction -> setTransactionName Jun 16, 2020

HazAT requested review from AbhiPrasad, dcramer, rhcarvalho and untitaker June 16, 2020 08:25

AbhiPrasad reviewed Jun 16, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

packages/hub/src/scope.ts Show resolved Hide resolved

rhcarvalho reviewed Jun 16, 2020

View reviewed changes

Update CHANGELOG.md

c327c03

Co-authored-by: Abhijeet Prasad <[email protected]>

untitaker reviewed Jun 16, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

packages/apm/src/span.ts Outdated Show resolved Hide resolved

packages/apm/src/span.ts Outdated Show resolved Hide resolved

packages/apm/test/hub.test.ts Outdated Show resolved Hide resolved

HazAT added 2 commits June 17, 2020 12:27

ref: CodeReview

045fd88

fix: Node

2d7ccba

rhcarvalho reviewed Jun 18, 2020

View reviewed changes

packages/hub/src/scope.ts Show resolved Hide resolved

packages/types/src/scope.ts Show resolved Hide resolved

packages/types/src/span.ts Outdated Show resolved Hide resolved

HazAT added 3 commits June 18, 2020 15:46

ref: CR

6779197

Merge branch 'master' into hazat/getTransaction

5ee0fe3

ref: Remove unused code

e0273ad

kamilogorek reviewed Jun 22, 2020

View reviewed changes

HazAT added 2 commits June 22, 2020 10:46

ref: Code Review

ebd3680

ref: Revert tests

fd23147

kamilogorek self-requested a review June 22, 2020 08:58

kamilogorek approved these changes Jun 22, 2020

View reviewed changes

HazAT merged commit eb4846e into master Jun 22, 2020

HazAT deleted the hazat/getTransaction branch June 22, 2020 09:02

HazAT mentioned this pull request Jun 23, 2020

feat: Add retrieving Transaction getsentry/sentry-docs#1775

Merged

rhcarvalho mentioned this pull request Jun 23, 2020

feat: Access transaction in current scope getsentry/sentry-python#734

Merged

AbhiPrasad mentioned this pull request Apr 5, 2022

[v7] feat(hub): Remove setTransaction scope method #4865

Merged

lobsterkatie mentioned this pull request Sep 12, 2022

Clean up code relating to scope/event transaction value #5718

Closed

Uh oh!

Comments

Conversation

HazAT commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

getsentry-bot commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

untitaker commented Jun 16, 2020

Uh oh!

dcramer commented Jun 16, 2020

Uh oh!

untitaker commented Jun 16, 2020

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamilogorek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

HazAT commented Jun 11, 2020 •

edited

Loading

getsentry-bot commented Jun 11, 2020 •

edited

Loading