feat(am): Introduce startTransaction

[HazAT]

• Fix Tests
• Fix Node

Motivation

The current SDK API can be very confusing when it comes to usage and it wasn't clear what the difference between a Transaction and Span is. This PR should align with the clarity and descriptions we now use in the AM docs getsentry/sentry-docs#1667

tl;dr

• We have a different object for Transaction / Span
• Users need to start a Transaction and finish it to send to Sentry (no more everything is startSpan with some properties)

Public API:

• Start a transaction: Sentry.startTransaction() → returns Transaction object
• Add spans to transaction: transactionObj.startChild() for a child span of the root or spanObj.startChild() for a child span of a lower-level span→ returns Span object
• Close a span: spanObj.finish()
• Close a transaction: transactionObj.finish()
• Set data on transaction or span: transactionObj.setX() or spanObj.setX()
• For browser:
  • Enable automatic capturing of page load transactions, XHR spans, Performance API spans, etc: ApmIntegrations.Tracing()
  • Enable automatic capturing of Vue component renders and lifecycle events: Integrations.VueIntegration({ Vue, tracing: true })
• For node:
  • Enable automatic capturing of requests as transactions: Integrations.Http({ tracing: true })
  • Enable automatic capturing of Express middleware: Apm.Integrations.Express({ app })

User Entry Point Sentry.startTransaction

const transaction = Sentry.startTransaction({
  name: 'my Custom Transaction',
  op: 'measure',
});
// transaction.setName('new transaction name');
const span1 = transaction.startChild({op: 'test'});
const span2 = span1.startChild({op: 'sub child'});
// Some heavy lifting
span2.finish();
span1.finish();
transaction.finish();

A span on the Scope

Previously it was the case that if a Span was on the Scope it was considered to be the Transaction / an entry point for hub.startSpan calls to add children to the span on the Scope.
This is a very magic and breaking behavior (what if there is suddenly no more span on the scope, what if it changes).

The way this PR utilizes this is that it considers whatever span is on Scope to take as the Trace / parent_span_id for newly created Transactions.

Right now the TracingIntegration starts a trace and sets an empty Span on the Scope (as the "trace"). With that, we make sure all succeeding Transactions are part of the same trace. The Integration keeps track of any ongoing Transaction itself. The Transaction and all children will have the same Trace ID.

Users should never interact with hub.startSpan

*Unless they really know what they are doing

The Entrypoint is meant to be Sentry.startTransaction only. From there they create child Spans. After they finish() the Transaction we flush it.

Node Usage

Can be found in this example https://github.com/kamilogorek/express-apm-playground

tl;dr for Express. User needs to import and use the @sentry/apm package.
We create a transaction and set it on the Scope and set a function on the Response called getTransaction. Users use getTransaction if they want to add child Spans to it.

Code:

const { Integrations } = require("@sentry/apm");
...

Sentry.init({
  tracesSampleRate: 1.0,
  integrations: [
    new Sentry.Integrations.Http({
      tracing: true
    }),
    // Express integration from APM package
    new Integrations.Express({ app })
  ],
});
...
app.use(Sentry.Handlers.requestHandler());
app.use(Sentry.Handlers.tracingHandler());

// User Code
app.get("/users", function usersHandler(req, res) {
  const transaction = res.getTransaction && res.getTransaction();
  const span = transaction && transaction.startChild({
      op: "encode",
      description: "parseAvatarImages"
    });
  }
  http.get("http://example.com/users", response => {
    response.on("data", () => {});
    response.on("end", () => {
      if (span) span.finish();
      res.status(200).end());
    });
  });
});
// End User Code

app.use(Sentry.Handlers.errorHandler());
app.listen(3000, () => { ....

Few other fixes

• Non Transaction events no longer contain contexts.trace
• TracingIntegration: Renamed discardBackgroundSpans to markBackgroundTransactions
• span.finish(endTimestamp?: number): void finish method receives optional endTimestamp (to be able to have another finish point than call time of the function [helpful in integrations])
• TransactionContext receives a trimEnd?: boolean property that is helpful for Browser and the IdleTransaction. This will be used to tell the Transaction to use the timestamp of the last Span instead of the timestamp of when finish() was called.
• Renamed properties in Span to reflect what they really are timestamp -> endTimestamp (JSON payload still looks the same)
• Add safeguard if a Transaction is created without a name property.
• Transaction events no longer go through beforeSend -> Motivation: This shouldn't have been there in the first place. If we ever want to move to individual Span ingestion, we can't users rely on using this for Transaction. Since Tracing is still considered beta, we are ok "breaking" this for now if users relied on that.

[lobsterkatie]

I'm excited we're doing this - I think it will make using tracing a lot easier! 🎉

First pass, just a bunch of suggestions wordsmithing docstrings and comments.

[getsentry-bot]

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

Generated by 🚫 dangerJS against 5446d82

[rhcarvalho]

• Renamed properties in Span to reflect what they really are timestamp -> endTimestamp (JSON payload still looks the same)

❤️

[rhcarvalho]

• transaction.finish(endTimestamp?: number, trimEnd?: boolean): string | undefined; transaction has an additional argument determining if it should use the endTimestamp of the last span instead of the one from its own finish call.

Thinking a bit about how those two arguments interact together and how people will use them.
I see we're using them like this:

active.finish(undefined, /*trimEnd*/ true);

IMHO that's a bit too smart code.

The reason to have trimEnd was the "idle transaction", and I remember @kamilogorek missing being able to set a specific finish time in the Vue integration, for which now there is a clear solution -- the endTimestamp argument.

Could we use the same explicit endTimestamp in the "idle transaction" case and clean up the API by removing trimEnd? The finish method signature would become the same for Transactions and Spans.

[kamilogorek]

Agree with Rodolfo. What I'd use instead is the options object (changing it for Span as well). It'd allow us to extend finish methods of both classes in the future if we ever need to, eg. "debounce", "wait" or any other ideas, as well as get rid of this random null, true or undefined, true thing

[dashed]

I'd be interested in alternatives to modifying transaction events if they're no longer reaching beforeSend().

The use case for this is in getsentry/sentry#18822

[rhcarvalho]

I don't want to go too much off-topic, but I also don't like this PR becoming a huge amount of changes all at once.

If this PR is changing the behavior of beforeSend, we may want to either consider the other parts of the Unified API that accidentally touch transactions because they are implemented on top of events, or move off the discussion/implementation changes to a separate PR (beforeSend is orthogonal to startTransaction, the original purpose of this PR).

I suspect the second option is easier.

• Transaction events no longer go through beforeSend -> Motivation: This shouldn't have been there in the first place. If we ever want to move to individual Span ingestion, we can't users rely on using this for Transaction. Since Tracing is still considered beta, we are ok "breaking" this for now if users relied on that.

There are other mechanisms in the SDKs that take in a raw "event".

• What about EventProcessors, are they called for transactions?
• What about BeforeBreadcrumb?
• What parts of the Scope apply to transactions?
• How do installed integrations interact with transactions?

(consider all those rhetorical questions for now, I'd rather discuss them separately instead of condensing all into one XXL PR)

[HazAT]

@dashed there is a solution to this problem. The thing is exactly for that reason we do not want to expose it so ppl don't rely on it. Otherwise we can probably never move to individual Span ingestion.

@rhcarvalho Then let's finish reviewing it so I can merge ^^
To answer your questions:

• Transactions still go through EventProcessors (they are considered internal and we can break this in the future more easily)
• Breadcrumbs != Transaction
• All of the Scope applies to Transactions
• With EventProcessors

[kamilogorek]

The PR as is in the current state is ready to be merged. We can further discuss any changes once we merge it, so that we don't prevent other work from happening.

[dashed]

@dashed there is a solution to this problem. The thing is exactly for that reason we do not want to expose it so ppl don't rely on it. Otherwise we can probably never move to individual Span ingestion.

I don't quite follow. 🤔 How does changing the transaction name on the transaction event affect individual Span ingestion in the future? Is there a spec that I can read to better understand the side effects?

[HazAT]

@dashed I will add you to the PR in Sentry.
The point is we don't want people to rely on beforeSend to receive Transaction, once we move to single Span ingestion this will break 100%.