feat: Add scope.getTransaction, rename scope.setTransaction -> setTransactionName

[HazAT]

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]

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

Generated by 🚫 dangerJS against fd23147

[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

[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()

[rhcarvalho]

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

[HazAT]

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.

[rhcarvalho]

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

[untitaker]

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

[dcramer]

@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]

@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).

[kamilogorek]

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().

[HazAT]

Moved to scope.test.ts

[kamilogorek]

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

[kamilogorek]

kilobytes, kilobytes 😶

[kamilogorek]

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.