Skip to content

feat(opentelemetry): Add new @sentry/opentelemetry package #9238

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 19, 2023
Merged

feat(opentelemetry): Add new @sentry/opentelemetry package #9238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .craft.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ targets:
- name: npm
id: '@sentry/replay'
includeNames: /^sentry-replay-\d.*\.tgz$/
## 1.6. OpenTelemetry package
- name: npm
id: '@sentry/opentelemetry'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prob also need to add this to the verdaccio config for the e2e tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not automatically be picked up there? 🤔 based on the namespace...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No unfortunately not:

sentry-javascript/packages/e2e-tests/verdaccio-config/config.yaml

Lines 29 to 39 in cadeefe

# To not use a proxy (e.g. npm) but instead use verdaccio for package hosting we need to define rules here without the
# `proxy` field. Sadly we can't use a wildcard like "@sentry/*" because we have some dependencies (@sentry/cli,
# @sentry/webpack-plugin) that fall under that wildcard but don't live in this repository. If we were to use that
# wildcard, we would get a 404 when attempting to install them, since they weren't uploaded to verdaccio, and also
# don't have a proxy configuration.
'@sentry/angular':
access: $all
publish: $all
unpublish: $all
# proxy: npmjs # Don't proxy for E2E tests!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn, good catch! I'll actually add this here as well, is missing there so far: https://github.com/getsentry/sentry-javascript/blob/develop/docs/new-sdk-release-checklist.md

includeNames: /^sentry-opentelemetry-\d.*\.tgz$/

## 2. Browser & Node SDKs
- name: npm
Expand Down
6 changes: 4 additions & 2 deletions docs/new-sdk-release-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ This page serves as a checklist of what to do when releasing a new SDK for the f

- [ ] Make sure it is added to `bundlePlugins.ts:makeTSPlugin` as `paths`, otherwise it will not be ES5 transpiled correctly for CDN builds.

- [ ] Make sure it is added to the [Verdaccio config](https://github.com/getsentry/sentry-javascript/blob/develop/packages/e2e-tests/verdaccio-config/config.yaml) for the E2E tests

## Cutting the Release

When you’re ready to make the first release, there are a couple of steps that need to be performed in the **correct order**. Note that you can prepare the PRs at any time but the **merging oder** is important:
Expand All @@ -56,7 +58,7 @@ When you’re ready to make the first release, there are a couple of steps that
### Before the Release:

- [ ] 1) If not yet done, be sure to remove the `private: true` property from your SDK’s `package.json`. Additionally, ensure that `"publishConfig": {"access": "public"}` is set.
- [ ] 2) Make sure that the new SDK is **not added** in`[craft.yml](https://github.com/getsentry/sentry-javascript/blob/master/.craft.yml)` as a target for the **Sentry release registry**\
- [ ] 2) Make sure that the new SDK is **not added** in`[craft.yml](https://github.com/getsentry/sentry-javascript/blob/develop/.craft.yml)` as a target for the **Sentry release registry**\
*Once this is added, craft will try to publish an entry in the next release which does not work and caused failed release runs in the past*
- [ ] 3) Add an `npm` target in `craft.yml` for the new package. Make sure to insert it in the right place, after all the Sentry dependencies of your package but before packages that depend on your new package (if applicable).
```yml
Expand All @@ -74,7 +76,7 @@ When you’re ready to make the first release, there are a couple of steps that
You have to fork this repo and PR the files from your fork to the main repo \
[Example PR](https://github.com/getsentry/sentry-release-registry/pull/80) from the Svelte SDK

- [ ] 2) Add an entry to `[craft.yml](https://github.com/getsentry/sentry-javascript/blob/master/.craft.yml)` to add releases of your SDK to the Sentry release registry \
- [ ] 2) Add an entry to [craft.yml](https://github.com/getsentry/sentry-javascript/blob/develop/.craft.yml) to add releases of your SDK to the Sentry release registry \
[Example PR](https://github.com/getsentry/sentry-javascript/pull/5547) from the Svelte SDK \
*Subsequent releases will now be added automatically to the registry*

Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"packages/node-integration-tests",
"packages/node-experimental",
"packages/opentelemetry-node",
"packages/opentelemetry",
"packages/react",
"packages/remix",
"packages/replay",
Expand Down
6 changes: 6 additions & 0 deletions packages/e2e-tests/verdaccio-config/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,12 @@ packages:
unpublish: $all
# proxy: npmjs # Don't proxy for E2E tests!

'@sentry/opentelemetry':
access: $all
publish: $all
unpublish: $all
# proxy: npmjs # Don't proxy for E2E tests!

'@sentry/react':
access: $all
publish: $all
Expand Down
2 changes: 1 addition & 1 deletion packages/node-experimental/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const span = Sentry.startSpan({ description: 'non-active span' });

doSomethingSlow();

span?.finish();
span.finish();
```

Finally you can also get the currently active span, if you need to do more with it:
Expand Down
36 changes: 18 additions & 18 deletions packages/node-experimental/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,26 @@
},
"dependencies": {
"@opentelemetry/api": "~1.6.0",
"@opentelemetry/context-async-hooks": "~1.17.0",
"@opentelemetry/core": "~1.17.0",
"@opentelemetry/instrumentation": "~0.43.0",
"@opentelemetry/instrumentation-express": "~0.33.1",
"@opentelemetry/instrumentation-fastify": "~0.32.3",
"@opentelemetry/instrumentation-graphql": "~0.35.1",
"@opentelemetry/instrumentation-http": "~0.43.0",
"@opentelemetry/instrumentation-mongodb": "~0.37.0",
"@opentelemetry/instrumentation-mongoose": "~0.33.1",
"@opentelemetry/instrumentation-mysql": "~0.34.1",
"@opentelemetry/instrumentation-mysql2": "~0.34.1",
"@opentelemetry/instrumentation-nestjs-core": "~0.33.1",
"@opentelemetry/instrumentation-pg": "~0.36.1",
"@opentelemetry/resources": "~1.17.0",
"@opentelemetry/sdk-trace-base": "~1.17.0",
"@opentelemetry/semantic-conventions": "~1.17.0",
"@prisma/instrumentation": "~5.3.1",
"@opentelemetry/core": "~1.17.1",
"@opentelemetry/context-async-hooks": "~1.17.1",
"@opentelemetry/instrumentation": "0.44.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hard-pinned all instrumentation dependencies, and pinned the "core" dependencies to bugfixes. LMK if you think we should hard pin everything 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is fine - we can always adjust it afterwards too if it ends up breaking something.

"@opentelemetry/instrumentation-express": "0.33.2",
"@opentelemetry/instrumentation-fastify": "0.32.3",
"@opentelemetry/instrumentation-graphql": "0.35.2",
"@opentelemetry/instrumentation-http": "0.44.0",
"@opentelemetry/instrumentation-mongodb": "0.37.1",
"@opentelemetry/instrumentation-mongoose": "0.33.2",
"@opentelemetry/instrumentation-mysql": "0.34.2",
"@opentelemetry/instrumentation-mysql2": "0.34.2",
"@opentelemetry/instrumentation-nestjs-core": "0.33.2",
"@opentelemetry/instrumentation-pg": "0.36.2",
"@opentelemetry/resources": "~1.17.1",
"@opentelemetry/sdk-trace-base": "~1.17.1",
"@opentelemetry/semantic-conventions": "~1.17.1",
"@prisma/instrumentation": "5.4.2",
"@sentry/core": "7.74.1",
"@sentry/node": "7.74.1",
"@sentry/opentelemetry-node": "7.74.1",
"@sentry/opentelemetry": "7.74.1",
"@sentry/types": "7.74.1",
"@sentry/utils": "7.74.1",
"opentelemetry-instrumentation-fetch-node": "1.1.0"
Expand Down
21 changes: 0 additions & 21 deletions packages/node-experimental/src/constants.ts
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions packages/node-experimental/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@ export { init } from './sdk/init';
export { INTEGRATIONS as Integrations };
export { getAutoPerformanceIntegrations } from './integrations/getAutoPerformanceIntegrations';
export * as Handlers from './sdk/handlers';
export * from './sdk/trace';
export { getActiveSpan } from './utils/getActiveSpan';
export { getCurrentHub, getHubFromCarrier } from './sdk/hub';
export type { Span } from './types';

export { startSpan, startInactiveSpan, getCurrentHub, getActiveSpan } from '@sentry/opentelemetry';

export {
makeNodeTransport,
defaultStackParser,
Expand Down
13 changes: 5 additions & 8 deletions packages/node-experimental/src/integrations/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,14 @@ import { SpanKind } from '@opentelemetry/api';
import { registerInstrumentations } from '@opentelemetry/instrumentation';
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { hasTracingEnabled, isSentryRequestUrl } from '@sentry/core';
import { _INTERNAL, getCurrentHub, getSpanKind, setSpanMetadata } from '@sentry/opentelemetry';
import type { EventProcessor, Hub, Integration } from '@sentry/types';
import { stringMatchesSomePattern } from '@sentry/utils';
import type { ClientRequest, IncomingMessage, ServerResponse } from 'http';

import { OTEL_ATTR_ORIGIN } from '../constants';
import { setSpanMetadata } from '../opentelemetry/spanData';
import type { NodeExperimentalClient } from '../sdk/client';
import { getCurrentHub } from '../sdk/hub';
import { getRequestSpanData } from '../utils/getRequestSpanData';
import type { NodeExperimentalClient } from '../types';
import { addOriginToSpan } from '../utils/addOriginToSpan';
import { getRequestUrl } from '../utils/getRequestUrl';
import { getSpanKind } from '../utils/getSpanKind';

interface HttpOptions {
/**
Expand Down Expand Up @@ -148,7 +145,7 @@ export class Http implements Integration {

/** Update the span with data we need. */
private _updateSpan(span: Span, request: ClientRequest | IncomingMessage): void {
span.setAttribute(OTEL_ATTR_ORIGIN, 'auto.http.otel.http');
addOriginToSpan(span, 'auto.http.otel.http');

if (getSpanKind(span) === SpanKind.SERVER) {
setSpanMetadata(span, { request });
Expand All @@ -161,7 +158,7 @@ export class Http implements Integration {
return;
}

const data = getRequestSpanData(span);
const data = _INTERNAL.getRequestSpanData(span);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: This _INTERNAL thing seems very smelly to me. Can we not hoist this into utils or smth?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine because it's dependent on us making some decisions around schema/naming. Internal is sufficiently scary enough that things shouldn't be used with it, and it will be going away very soon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so this should definitely go away, I just want to avoid exposing things publicly that we need right now because then we can't remove them easily later :) once the semantic attributes stuff is merged and agreed upon this can/should just be public API!

getCurrentHub().addBreadcrumb(
{
category: 'http',
Expand Down
12 changes: 5 additions & 7 deletions packages/node-experimental/src/integrations/node-fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,12 @@ import type { Span } from '@opentelemetry/api';
import { SpanKind } from '@opentelemetry/api';
import type { Instrumentation } from '@opentelemetry/instrumentation';
import { hasTracingEnabled } from '@sentry/core';
import { _INTERNAL, getCurrentHub, getSpanKind } from '@sentry/opentelemetry';
import type { Integration } from '@sentry/types';
import { FetchInstrumentation } from 'opentelemetry-instrumentation-fetch-node';

import { OTEL_ATTR_ORIGIN } from '../constants';
import type { NodeExperimentalClient } from '../sdk/client';
import { getCurrentHub } from '../sdk/hub';
import { getRequestSpanData } from '../utils/getRequestSpanData';
import { getSpanKind } from '../utils/getSpanKind';
import type { NodeExperimentalClient } from '../types';
import { addOriginToSpan } from '../utils/addOriginToSpan';
import { NodePerformanceIntegration } from './NodePerformanceIntegration';

interface NodeFetchOptions {
Expand Down Expand Up @@ -101,7 +99,7 @@ export class NodeFetch extends NodePerformanceIntegration<NodeFetchOptions> impl

/** Update the span with data we need. */
private _updateSpan(span: Span): void {
span.setAttribute(OTEL_ATTR_ORIGIN, 'auto.http.otel.node_fetch');
addOriginToSpan(span, 'auto.http.otel.node_fetch');
}

/** Add a breadcrumb for outgoing requests. */
Expand All @@ -110,7 +108,7 @@ export class NodeFetch extends NodePerformanceIntegration<NodeFetchOptions> impl
return;
}

const data = getRequestSpanData(span);
const data = _INTERNAL.getRequestSpanData(span);
getCurrentHub().addBreadcrumb({
category: 'http',
data: {
Expand Down
37 changes: 0 additions & 37 deletions packages/node-experimental/src/opentelemetry/contextManager.ts
View file
Open in desktop

This file was deleted.

99 changes: 0 additions & 99 deletions packages/node-experimental/src/opentelemetry/spanProcessor.ts
View file
Open in desktop

This file was deleted.

Loading