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

Tracing without Performance #8352

Closed
23 of 25 tasks
Tracked by #5
stephanie-anderson opened this issue Jun 19, 2023 · 5 comments
Closed
23 of 25 tasks
Tracked by #5

Tracing without Performance #8352

stephanie-anderson opened this issue Jun 19, 2023 · 5 comments

Comments

@stephanie-anderson
Copy link
Contributor

stephanie-anderson commented Jun 19, 2023

Problem Statement

It should be possible that tracing information is propagated from/to incoming/outgoing HTTP requests even if performance is disabled and thus no transactions/spans are available.

Solution Brainstorm

Requirements:

  1. All error, transaction and check-in events sent to Sentry always contain a Dynamic Sampling Context (trace) envelope header item.
  2. All error, transaction and check-in events sent to Sentry always contain a trace context.
  3. All outgoing HTTP requests always contain a sentry-trace and baggage header if the destination is allow-listed in [tracePropagationTargets](https://develop.sentry.dev/sdk/performance/#tracepropagationtargets).

Part 1: Standardize tracePropagationTargets

  1. AbhiPrasad
  2. AbhiPrasad
  3. AbhiPrasad

Part 3: Tracing without Performance

  1. AbhiPrasad
  2. AbhiPrasad
  3. AbhiPrasad
  4. AbhiPrasad
  5. AbhiPrasad
  6. 0 of 3
    Type: Improvement

Part 5: Final touches

  1. AbhiPrasad
  2. AbhiPrasad
@AbhiPrasad
Copy link
Member

We might want to explore if it's worth it to create a separate integration for browser JS that enables distributed tracing, but not take the bundle size hit of all of performance monitoring.

@danielkhan
Copy link

We might want to explore if it's worth it to create a separate integration for browser JS that enables distributed tracing, but not take the bundle size hit of all of performance monitoring.

Each dedicated package increases complexity for us but also for our users.
Additionally, it makes it harder to transition to performance as they have to change the SDK.

If the difference isn't really game-changing significant, I would not do that right now.

@AbhiPrasad
Copy link
Member

Each dedicated package increases complexity for us but also for our users.

This wouldn't be a dedicated package, but instead just a different integration exported from Sentry. Swapping them would be as easy as removing one integration and using another.

import * as Sentry from '@sentry/browser';

Sentry.init({
  integrations: [
    // Tracing + Performance Monitoring
    // Sentry.BrowserTracing(),
    //
    // Just Tracing (name tbd, hardest problem in computer science and all of that)
    // Sentry.PlainTracing
  ],
});

AbhiPrasad added a commit that referenced this issue Jun 27, 2023
ref #8352

As we work toward adding tracing without performance support, this PR
updates the `BrowserTracing` integration to use and favour the top level
`tracePropagationTargets` option if it exists.

This option was made top level in
#8395

`tracePropagationTargets` is now part of the unified API for distributed
tracing. It's also expected that electron/react native will behave the
same way as well. This also leaves us the flexibility to extract tracing
out of BrowserTracing, or create a new integration that just does
tracing but no performance monitoring.

We can make sure this migration is smooth and easy to understand with a
good set of docs, which is what I will be working on next. In these docs
changes, we'll be updating the automatic instrumentation sections, and
formally documented `tracePropagationTargets` as a high level option.
AbhiPrasad added a commit that referenced this issue Jun 29, 2023
ref #8352

For more details about PropagationContext, see
https://www.notion.so/sentry/Tracing-without-performance-efab307eb7f64e71a04f09dc72722530

Building off of work in both
#8403 and
#8418, this PR adds
`PropagationContext` and uses that to always set a trace context on
outgoing error events.

Currently if there is an active span on the scope, we automatically
attach that span's trace context to all outgoing events. Now, we want to
rely on either the active span or fallback to the propagation context to
ensure that there is always a trace being generated and propagated.

Next up we'll work on updating the node/browser SDKs to update the
propagation context. For example, we should update the propagation
context for node based on the incoming sentry-trace/baggage headers.
AbhiPrasad added a commit that referenced this issue Jun 29, 2023
Requires #8421 to be
merged

ref #8352

This PR adds support for
https://github.com/getsentry/rfcs/blob/main/text/0071-continue-trace-over-process-boundaries.md
via propagation context. When the Node SDK initializes, it grabs
`process.env.SENTRY_TRACE` and `process.env.SENTRY_BAGGAGE` and uses
them to populate the existing propagation context.

In future PRs (tracked by #8352), we will be adding support for
transactions/outgoing requests to use propagation context, but for now
we are only storing in propagation context, not doing anything with it.
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jul 12, 2023

Initial support released with https://github.com/getsentry/sentry-javascript/releases/tag/7.58.0

I opened #8520 to extract task for meta tags.

All thats left now is to merge in docs (getsentry/sentry-docs#7421) and to write down key learnings so that we can add twp support to react native and electron.

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jul 17, 2023

For electron and react native:

To add tracing without performance support on top of the JS SDKs, you can do the following:

if (shouldAttachTraceData(rawRequestUrl)) {
if (requestSpan) {
const sentryTraceHeader = requestSpan.toTraceparent();
const dynamicSamplingContext = requestSpan?.transaction?.getDynamicSamplingContext();
addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, dynamicSamplingContext);
} else {
const client = hub.getClient();
const { traceId, sampled, dsc } = scope.getPropagationContext();
const sentryTraceHeader = generateSentryTraceHeader(traceId, undefined, sampled);
const dynamicSamplingContext =
dsc || (client ? getDynamicSamplingContextFromClient(traceId, client, scope) : undefined);
addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, dynamicSamplingContext);
}
} else {
__DEBUG_BUILD__ &&
logger.log(
`[Tracing] Not adding sentry-trace header to outgoing request (${requestUrl}) due to mismatching tracePropagationTargets option.`,
);
}

Essentially if a span does not exist (because no performance or span not sampled), then grab tracing information from scope.getPropagationContext and generate a sentry-trace and baggage header accordingly.

Closing this issue because docs were merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants