fix(apm): Sampling of traces (#2500)

* fix(apm): Set sampled to true by default Also set op otherwise span will be discarded by the server * meta: Changelog * fix(apm): Sampling * ref: Remove set default op * fix: Sampling decision * ref: Add comment * ref: typo * ref: Changes to docblock * ref: Change message * Apply suggestions from code review Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io> * Update packages/types/src/options.ts Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io> * Update packages/types/src/options.ts Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io> * ref: Remove deprecated parts * fix: Maxlen * ref: Rework when and how integrations are setup * ref(apm): Send a span if it's not a child * ref: Tracing integration * fix: tests * fix: Span / Transaction creation * ref: CodeReview * fix: Setup integrations when after we bound a client to the hub * ref: CodeReview * fix: tests * Update packages/types/src/span.ts Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io> * ref: CodeReview * ref: CodeReview * fix: Tests * ref: Refactor SpanRecorder -> SpanList Fix tests * ref: Rename back to SpanRecorder to be consistent with Python * ref: SpanRecorder * ref: Remove makeRoot * ref: Changelog * meta: Changelog Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
getsentry · Mar 19, 2020 · dea97aa · dea97aa
1 parent 39af3b4
commit dea97aa
Show file tree

Hide file tree

Showing 15 changed files with 404 additions and 233 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,12 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+- [apm] fix: Sampling of traces work now only depending on the client option `tracesSampleRate` (#2500)
+- [apm] fix: Remove internal `forceNoChild` parameter from `hub.startSpan` (#2500)
+- [apm] fix: Made constructor of `Span` internal, only use `hub.startSpan` (#2500)
+- [apm] ref: Remove status from tags in transaction (#2497)
+- [browser] fix: Respect breadcrumbs sentry:false option (#2499)
+
 ## 5.14.2
 
 - [apm] fix: Use Performance API for timings when available, including Web Workers (#2492)

diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts
@@ -1,17 +1,8 @@
 import { getMainCarrier, Hub } from '@sentry/hub';
 import { SpanContext } from '@sentry/types';
-import { isInstanceOf } from '@sentry/utils';
 
 import { Span } from './span';
 
-/**
- * Checks whether given value is instance of Span
- * @param span value to check
- */
-function isSpanInstance(span: unknown): span is Span {
-  return isInstanceOf(span, Span);
-}
-
 /** Returns all trace headers that are currently on the top scope. */
 function traceHeaders(): { [key: string]: string } {
   // @ts-ignore
@@ -33,36 +24,42 @@ function traceHeaders(): { [key: string]: string } {
  * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope,
  * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`.
  *
- * @param span Already constructed span which should be started or properties with which the span should be created
+ * @param spanContext Already constructed span or properties with which the span should be created
  */
-function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean = false): Span {
+function startSpan(spanContext?: SpanContext): Span {
   // @ts-ignore
-  const that = this as Hub;
-  const scope = that.getScope();
-  const client = that.getClient();
+  const hub = this as Hub;
+  const scope = hub.getScope();
+  const client = hub.getClient();
   let span;
 
-  if (!isSpanInstance(spanOrSpanContext) && !forceNoChild) {
-    if (scope) {
-      const parentSpan = scope.getSpan() as Span;
-      if (parentSpan) {
-        span = parentSpan.child(spanOrSpanContext);
-      }
+  // This flag determines if we already added the span as a child to the span that currently lives on the scope
+  // If we do not have this, we will add it later on twice to the span recorder and therefore have too many spans
+  let addedAsChild = false;
+
+  if (scope) {
+    const parentSpan = scope.getSpan() as Span;
+    if (parentSpan) {
+      span = parentSpan.child(spanContext);
+      addedAsChild = true;
     }
   }
 
-  if (!isSpanInstance(span)) {
-    span = new Span(spanOrSpanContext, that);
+  if (!span) {
+    span = new Span(spanContext, hub);
   }
 
-  if (span.sampled === undefined && span.transaction !== undefined) {
+  // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state
+  if (span.sampled === undefined && span.isRootSpan()) {
     const sampleRate = (client && client.getOptions().tracesSampleRate) || 0;
     span.sampled = Math.random() < sampleRate;
   }
 
-  if (span.sampled) {
+  // We only want to create a span list if we sampled the transaction
+  // in case we will discard the span anyway because sampled == false, we safe memory and do not store child spans
+  if (span.sampled && !addedAsChild) {
     const experimentsOptions = (client && client.getOptions()._experiments) || {};
-    span.initFinishedSpans(experimentsOptions.maxSpans as number);
+    span.initSpanRecorder(experimentsOptions.maxSpans as number);
   }
 
   return span;