10000 feat(node): Export tracing from `@sentry/node` by timfish · Pull Request #7503 · getsentry/sentry-javascript · GitHub
[go: up one dir, main page]
Skip to content

feat(node): Export tracing from @sentry/node #7503

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

Jump to bottom
Merged
merged 18 commits into from
Mar 21, 2023
Merged

feat(node): Export tracing from @sentry/node #7503

merged 18 commits into from
Mar 21, 2023

Conversation

timfish
Copy link
Collaborator
@timfish timfish commented Mar 17, 2023 •
edited
Loading

The vast majority of the additions in this PR are where packages/node-integration-tests/suites/tracing/* has been copied to packages/node-integration-tests/suites/tracing-new/*. This is a complete copy of the tests but using the new API. At v8 we can drop the old tests that use @sentry/tracing.

This PR:

  • Adds @sentry-internal/tracing as a dependency of @sentry/node
  • From @sentry/node, exports all node tracing integrations under the existing Integrations export
  • Calls addTracingExtensions from the node client
  • Add autoDiscoverNodePerformanceMonitoringIntegrations which copies the logic from _autoloadDatabaseIntegrations but returns the integrations. It also includes some extra integrations that were not included in auto detection previously. Part of the code (lazyLoadedNodePerformanceMonitoringIntegrations) has to stay in @sentry-internal/tracing so the integrations can be lazily required.

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 17, 2023
View reviewed changes
packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts
});

const connection = mysql.createConnection({
user: 'root',

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "root" is used as [user name](1).
packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts

const connection = mysql.createConnection({
user: 'root',
password: 'docker',

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "docker" is used as [password](1).
@timfish

This comment was marked as outdated.

Sign in to view
@AbhiPrasad
Copy link
Member

@timfish maybe we just call addTracingExtensions by default in the Node client - I don't think we worry too much about tree-shaking it away in the Node side.

@timfish
Copy link
Collaborator Author
timfish commented Mar 17, 2023 •
edited
Loading

I don't think we worry too much about tree-shaking it away in the Node side.

Yep, true. I think it's < 10KB.

@timfish timfish marked this pull request as ready for review March 20, 2023 11:35
AbhiPrasad
AbhiPrasad reviewed Mar 20, 2023
View reviewed changes
@timfish
Copy link
Collaborator Author
timfish commented Mar 21, 2023

There's an intermittent test failure locally for:
OnUncaughtException integration - should close process on uncaught error with no additional listeners registered

I think this could be related to the fact that we now always call registerErrorInstrumentation which means there is always an extra uncaughtException listener.

@AbhiPrasad
Copy link
Member

I think this could be related to the fact that we now always call registerErrorInstrumentation which means there is always an extra uncaughtException listener.

Maybe we just need to call removeAllListeners()? We need to add a note about what is going on though.

@timfish
Copy link
Collaborator Author
timfish commented Mar 21, 2023 •
edited
Loading

I think the issue is that we try and replicate node.js default behaviour of exiting when no listeners are attached apart from Sentry's own listeners. The tracing error handler would have always broken this behaviour (so this is an existing bug) but now we add the tracing listener by default this test has started failing.

The tests are passing in CI so fix the listener exit bug in another PR?

@AbhiPrasad
Copy link
Member

I think the issue is that we try and replicate node.js default behaviour of exiting when no listeners are attached apart from Sentry's own listeners. The tracing error handler would have always broken this behaviour (so this is an existing bug) but now we add the tracing listener by default this test has started failing.

Ahhh gotcha, yeah we need to fix then.

The tests are passing in CI so fix the listener exit bug in another PR?

Sounds fine to me.

@AbhiPrasad AbhiPrasad merged commit 95b0a6c into getsentry:develop Mar 21, 2023
@AbhiPrasad AbhiPrasad mentioned this pull request Mar 21, 2023
Closed
@timfish timfish deleted the feat/node/tracing branch March 21, 2023 19:16
@timfish timfish mentioned this pull request Mar 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timfish @AbhiPrasad
0