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

[v8] Update span interface to new interface #10677

Closed
12 tasks done
mydea opened this issue Feb 15, 2024 · 11 comments
Closed
12 tasks done

[v8] Update span interface to new interface #10677

mydea opened this issue Feb 15, 2024 · 11 comments
Assignees
Milestone

Comments

@mydea
Copy link
Member

mydea commented Feb 15, 2024

We deprecated all the stuff on the span interface that will be removed in v8, now it's time to actually remove this!

@AbhiPrasad AbhiPrasad self-assigned this Feb 15, 2024
@AbhiPrasad AbhiPrasad added this to the 8.0.0 milestone Feb 16, 2024
AbhiPrasad added a commit that referenced this issue Feb 20, 2024
AbhiPrasad added a commit that referenced this issue Feb 21, 2024
ref #10677

Removes all usage of setting/getting `span.description` and `span.name`,
and removes the `span.setName` method.
AbhiPrasad added a commit that referenced this issue Feb 21, 2024
ref #10677

Removes `span.origin` as a getter.
AbhiPrasad added a commit that referenced this issue Feb 21, 2024
ref #10677

Removes `span.origin` as a getter.
AbhiPrasad added a commit that referenced this issue Feb 23, 2024
ref #10677

Removes `instrumenter` from the span interface, and removes the
`instrumenter` option from client options accordingly.
@gajus
Copy link
Contributor

gajus commented Feb 27, 2024

@mydea Any ETA for when v8 comes out?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 27, 2024
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Feb 27, 2024

@gajus first alpha comes out some time this week! But we're taking our time to make sure the API makes sense and the migration path is as smooth soon as possible.

Here's what is planned for first alpha in terms of migration - https://github.com/getsentry/sentry-javascript/blob/3b7af663db9e8a3a5567bcfcffbf1eb6208cf9ec/MIGRATION.md

@gajus
Copy link
Contributor

gajus commented Feb 27, 2024

@AbhiPrasad Reading these docs, it seems that you are abstracting all of the OpenTelemetry initialization logic away from the user and setting it up behind the scenes? How would one extend OpenTelemetry when Sentry is not the only product that they are using it with?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 27, 2024
@AbhiPrasad
Copy link
Member

How would one extend OpenTelemetry when Sentry is not the only product that they are using it with

We'll expose an API for this, just tbd what that looks like, first alpha is just to validate the e2e flow before we do that.

@gajus
Copy link
Contributor

gajus commented Feb 27, 2024

What was the rationale behind abstracting OpenTelemetry?

As an existing user of OpenTelemetry, it makes me feel uneasy that I am forced to decide between using Sentry v8 and using OpenTelemetry directly.

Furthermore, your design does little to educate users about the underlying OpenTelemetry tracing APIs and pushes towards using Sentry's API, which again feels counter-productive to helping the open-standard adoption.

I should also add performance concerns. We use @sentry/node in performance sensitive environments, and it looks like the proposed design does not leave a hatch for opting-out from OpenTelemetry instrumentation.

I strongly believe that Sentry should not intialize OpenTelemetry, and leave that part to the end user while guiding them through the setup.

Correct me if I am wrong, but the necessary initialization is limited to more or less the following:

const SentryContextManager = wrapContextManagerClass(
  AsyncLocalStorageContextManager,
);

const sdk = new opentelemetry.NodeSDK({
  contextManager: new SentryContextManager(),
  instrumentations: [
    getNodeAutoInstrumentations({
      // Disabling this to avoid hitting ENOENT errors
      '@opentelemetry/instrumentation-fs': {
        enabled: false,
      },
    }),
  ],
  sampler: new SentrySampler(client),
  spanProcessor: new SentrySpanProcessor(),
  textMapPropagator: new SentryPropagator(),
  traceExporter: new OTLPTraceExporter(),
});

// Ensure OpenTelemetry Context & Sentry Hub/Scope is synced
setOpenTelemetryContextAsyncContextStrategy();

sdk.start();

This is just a few extra lines of code to copy, but it means that the code owner retains full control over when and how OpenTelemetry is initialized.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 27, 2024
@mydea
Copy link
Member Author

mydea commented Feb 28, 2024

Hello,

you are totally free to use the plain OpenTelemetry APIs in v8 with @sentry/node! In fact, you'll be able to do const otelTracer = getClient().tracer and do with it whatever you want. You'll also be able to register your own instrumentation etc. So you can use OTEL-only APIs everywhere in your codebase, if you want that!

The reason we provide some layers over it is in order to provide some helpful things to users. You don't have to use those if you don't want to! You can do otelTracer.startActiveSpan('my-span') and everything should work just fine.

One key reason to have our own APIs is that it is important to us to have the same API in browser & server SDKs - especially for isomorphic frameworks like Next.js this is essential to us. And in the Browser SDK, we do not have OpenTelemetry (yet - for various reasons, the biggest being massively increased bundle size), so we need a shared set of APIs that Sentry SDK users can use in any environment.

Sadly, the code to setup OpenTelemetry and sync it with Sentry is not as straightforward - several steps have to be in place in order to ensure that errors captured from Sentry & traces captured via OpenTelemetry are always in sync. Because of this, it is not really feasible to make this optional - we opted to go all-in on OpenTelemetry and utilize it as much as possible in v8 of the JavaScript SDKs.

Continuing to provide a non-OTEL option is not a priority right now, especially because that would mean maintaining separate performance instrumentations in parallel, which is a lot of overhead. If you really don't want to use OpenTelemetry (e.g. for errors only), you can opt to use e.g. @sentry/server-runtime which is a more minimal variant without OpenTelemetry and most auto instrumentation for performance.

@gajus
Copy link
Contributor

gajus commented Feb 28, 2024

The biggest reason for wanting to setup OpenTelemetry ourselves is because we send traces to different backends depending on the environment in which they are used. How would this work if Sentry does the initialization?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 28, 2024
@mydea
Copy link
Member Author

mydea commented Feb 29, 2024

You'll still be able to setup other exporters etc.! And if something is really not working, we'll provide a way to opt out of the default setup and allow you to configure it yourself.

You'll be able to do getClient().traceProvider to access the provider we setup, then you can do e.g. traceProvider.addSpanProcessor(), for example.

@gajus
Copy link
Contributor

gajus commented Feb 29, 2024

Understood. Looking forward to trying it out. Thank you for the update.

@joekhoobyar
Copy link

Anybody who finds this issue in their travels, here is the sentry docs for using existing OpenTelemetry setups with Sentry:

If you already have OpenTelemetry set up yourself, you can also use your existing setup.

@AbhiPrasad
Copy link
Member

@joekhoobyar these are also published with the docs site: https://docs.sentry.io/platforms/javascript/guides/node/tracing/instrumentation/opentelemetry/

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

No branches or pull requests

5 participants