Skip to content

fix(db): Allow connection spans without a parent #25941

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

Draft
nicohrubec wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

fix(db): Allow connection spans without a parent #25941

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
2 changes: 1 addition & 1 deletion dev-packages/node-integration-tests/suites/tracing/knex/mysql2/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describeWithDockerCompose('knex auto instrumentation', { workingDirectory: [__di
await createRunner()
.expect({
span: container => {
expect(container.items.find(span => span.is_segment)?.name).toBe('Test Transaction');
expect(container.items.find(span => span.is_segment && span.name === 'Test Transaction')).toBeDefined();
const knexSpans = container.items.filter(span => span.attributes[SENTRY_ORIGIN]?.value === ORIGIN);
expect(knexSpans).toEqual(EXPECTED_SPANS);
},
Expand Down
4 changes: 2 additions & 2 deletions dev-packages/node-integration-tests/suites/tracing/mysql2-tracing-channel/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('mysql2 tracing channel Test', () => {
await createTestRunner()
.expect({
span: container => {
expect(container.items.find(span => span.is_segment)?.name).toBe('Test Transaction');
expect(container.items.find(span => span.is_segment && span.name === 'Test Transaction')).toBeDefined();
const spans = container.items.filter(span => !span.is_segment);
// The monkey-patch path (origin `auto.db.mysql2`) must be inactive on 3.20.0+.
expect(spans.find(span => span.attributes[SENTRY_ORIGIN]?.value === 'auto.db.mysql2')).toBeUndefined();
Expand All @@ -84,7 +84,7 @@ describe('mysql2 tracing channel Test', () => {
await createTestRunner()
.expect({
span: container => {
expect(container.items.find(span => span.is_segment)?.name).toBe('Test Transaction');
expect(container.items.find(span => span.is_segment && span.name === 'Test Transaction')).toBeDefined();
const spans = container.items.filter(span => !span.is_segment);
for (const span of spans) {
const queryText = span.attributes[DB_QUERY_TEXT]?.value;
Expand Down
3 changes: 1 addition & 2 deletions dev-packages/node-integration-tests/suites/tracing/postgres/scenario-no-parent.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ async function run() {
await probe.end();
});

// No active span here: `requireParentSpan` means the connect and this query
// must NOT produce spans.
// Connect can produce a root span; the query still requires a parent.
await client.connect();
await client.query('SELECT 1 AS unparented');

Expand Down
20 changes: 11 additions & 9 deletions dev-packages/node-integration-tests/suites/tracing/postgres/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,9 @@ describeWithDockerCompose('postgres auto instrumentation', { workingDirectory: [
.expect({
span: container => {
expect(container).toMatchObject(EXPECTED_SPANS);
const dbSpans = container.items.filter(span => span.attributes[SENTRY_OP]?.value === DB);
const dbSpans = container.items.filter(
span => !span.is_segment && span.attributes[SENTRY_OP]?.value === DB,
);
expect(dbSpans.map(span => span.name)).toEqual([
'pg.connect',
'CREATE TABLE "User"',
Expand Down Expand Up @@ -375,16 +377,15 @@ describeWithDockerCompose('postgres auto instrumentation', { workingDirectory: [

describe('requireParentSpan', () => {
createEsmAndCjsTests(__dirname, 'scenario-no-parent.mjs', 'instrument.mjs', (createTestRunner, test) => {
test('does not instrument queries or connects without an active parent span', { timeout: 90_000 }, async () => {
test('does not instrument queries without an active parent span', { timeout: 90_000 }, async () => {
await createTestRunner()
.expect({
span: container => {
const descriptions = container.items.map(span => span.name);
// The unparented connect + query must not have produced spans
// The unparented query must not have produced a span
expect(container.items.map(span => span.attributes[DB_QUERY_TEXT]?.value)).not.toContain(
'SELECT ? AS unparented',
);
expect(descriptions.find(name => name?.includes('connect'))).toBeUndefined();
expect(container.items.find(span => span.is_segment && span.name === 'pg.connect')).toBeDefined();
// Only the parented query is instrumented
expect(container).toMatchObject({
items: expect.arrayContaining([
Expand Down Expand Up @@ -521,7 +522,9 @@ describeWithDockerCompose('postgres auto instrumentation', { workingDirectory: [
.expect({
span: container => {
expect(container).toMatchObject(EXPECTED_SPANS);
const dbSpans = container.items.filter(span => span.attributes[SENTRY_OP]?.value === DB);
const dbSpans = container.items.filter(
span => !span.is_segment && span.attributes[SENTRY_OP]?.value === DB,
);
expect(dbSpans.map(span => span.name)).toEqual([
'pg.connect',
'CREATE TABLE "NativeUser"',
Expand Down Expand Up @@ -679,17 +682,16 @@ describeWithDockerCompose('postgres auto instrumentation', { workingDirectory: [
'instrument-orchestrion.mjs',
(createTestRunner, test) => {
test(
'does not instrument queries or connects without an active parent span',
'does not instrument queries without an active parent span (orchestrion)',
{ timeout: 90_000 },
async () => {
await createTestRunner()
.expect({
span: container => {
const descriptions = container.items.map(span => span.name);
expect(container.items.map(span => span.attributes[DB_QUERY_TEXT]?.value)).not.toContain(
'SELECT ? AS unparented',
);
expect(descriptions.find(name => name?.includes('connect'))).toBeUndefined();
expect(container.items.find(span => span.is_segment && span.name === 'pg.connect')).toBeDefined();
expect(container).toMatchObject({
items: expect.arrayContaining([
expect.objectContaining({ name: 'Test Transaction', is_segment: true }),
Expand Down
11 changes: 8 additions & 3 deletions dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory

createEsmAndCjsTests(__dirname, 'scenario-ioredis.mjs', 'instrument-ioredis.mjs', (createTestRunner, test) => {
test('should create cache spans for prefixed keys (ioredis)', { timeout: 60_000 }, async () => {
await createTestRunner().expect({ transaction: EXPECTED_TRANSACTION }).start().completed();
await createTestRunner().unordered().expect({ transaction: EXPECTED_TRANSACTION }).start().completed();
});
});
});
Expand Down Expand Up @@ -238,9 +238,14 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
.withEnv({ STREAMED: 'true' })
.expect({
span: container => {
expect(container.items.find(item => item.is_segment)?.name).toBe(segmentName);
expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([
'connect',
segmentName,
]);

expect(childSpans(container)).toEqual([
expect(
childSpans(container).filter(item => item.attributes[SENTRY_SEGMENT_NAME]?.value === segmentName),
).toEqual([
span('set localhost:6383', redisSpanOp, {
[DB_OPERATION_NAME]: 'set',
[DB_QUERY_TEXT]: 'set test-key [1 other arguments]',
Expand Down
12 changes: 9 additions & 3 deletions dev-packages/node-integration-tests/suites/tracing/redis/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,16 @@ describeWithDockerCompose('redis auto instrumentation', { workingDirectory: [__d
await createTestRunner()
.expect({
span: container => {
const segmentSpan = container.items.find(item => item.is_segment);
expect(segmentSpan?.name).toBe('Test Span');
expect(container.items.filter(item => item.is_segment).map(item => item.name)).toEqual([
'connect',
'Test Span',
]);

const dbSpans = container.items.filter(item => item.attributes[SENTRY_OP]?.value === redisSpanOp);
const dbSpans = container.items.filter(
item =>
item.attributes[SENTRY_OP]?.value === redisSpanOp &&
item.attributes[SENTRY_SEGMENT_NAME]?.value === 'Test Span',
);

expect(dbSpans).toEqual([
expectedDbSpan({ operation: 'set', statement: 'set test-key [1 other arguments]' }),
Expand Down
1 change: 0 additions & 1 deletion packages/server-utils/src/integrations/mysql2/mysql2-dc-subscriber.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ function setupConnectChannel(tracingChannel: MySQL2TracingChannelFactory, channe
bindTracingChannelToSpan(tracingChannel<MySQL2ConnectData>(channelName), data => {
return startInactiveSpan({
name: spanName,
onlyIfParent: true,
attributes: {
[SENTRY_ORIGIN]: ORIGIN,
[SENTRY_OP]: DB,
Expand Down
2 changes: 1 addition & 1 deletion packages/server-utils/src/integrations/postgres.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ function subscribeQueryLikeChannel(
const spanOptions = getSpanOptions(data);
return startInactiveSpan({
...spanOptions,
onlyIfParent: true,
onlyIfParent: channelName !== CHANNELS.PG_CONNECT && channelName !== CHANNELS.PGPOOL_CONNECT,
attributes: { ...spanOptions.attributes, [SENTRY_KIND]: 'client' },
});
},
Expand Down
1 change: 0 additions & 1 deletion packages/server-utils/src/integrations/redis/ioredis-channel-subscriber.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ export function instrumentIoredis(options: RedisCacheOptions): void {
const { host, port } = getConnectionOptions(data.self);
return startInactiveSpan({
name: 'connect',
onlyIfParent: true,
attributes: {
[SENTRY_KIND]: 'client',
...connectionAttributes(host, port),
Expand Down
25 changes: 17 additions & 8 deletions packages/server-utils/test/integrations/mysql2/mysql2-dc-subscriber.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ function installTestAsyncContextStrategy(): void {

/**
* Drives a channel's `tracePromise` inside an enclosing span and captures the span bound by the
* subscriber. The subscriber only creates a recording span when there is an enclosing active span, so the
* helper always establishes one and returns its id for parenting assertions.
* subscriber, returning the enclosing span's id for parenting assertions.
*/
async function traceOperation(
channelName: string,
Expand Down Expand Up @@ -350,12 +349,17 @@ describe('subscribeMysql2DiagnosticChannels', () => {

describe('connect channels', () => {
it('creates a connect span without db.query.text', async () => {
const { span } = await traceOperation(
MYSQL2_DC_CHANNEL_CONNECT,
let span: Span | undefined;
await tracingChannel(MYSQL2_DC_CHANNEL_CONNECT).tracePromise(
async () => {
span = getActiveSpan();
expect(span?.isRecording()).toBe(true);
},
{ database: 'test', serverAddress: '127.0.0.1', serverPort: 3306, user: 'root' },
{ result: undefined },
);

expect(spanToJSON(span!).parent_span_id).toBeUndefined();

const json = spanToJSON(span!);
expect(json.name).toBe('mysql2.connect');
expect(json.attributes['sentry.op']).toBe('db');
Expand All @@ -368,12 +372,17 @@ describe('subscribeMysql2DiagnosticChannels', () => {
});

it('names the pool connect span distinctly', async () => {
const { span } = await traceOperation(
MYSQL2_DC_CHANNEL_POOL_CONNECT,
let span: Span | undefined;
await tracingChannel(MYSQL2_DC_CHANNEL_POOL_CONNECT).tracePromise(
async () => {
span = getActiveSpan();
expect(span?.isRecording()).toBe(true);
},
{ database: 'test', serverAddress: '127.0.0.1', serverPort: 3306 },
{ result: undefined },
);

expect(spanToJSON(span!).parent_span_id).toBeUndefined();

expect(spanToJSON(span!).name).toBe('mysql2.pool.connect');
});

Expand Down
2 changes: 2 additions & 0 deletions packages/server-utils/test/integrations/postgres/postgres.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ describe('postgresIntegration', () => {
expect(startInactiveSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
name: 'pg.connect',
onlyIfParent: false,
attributes: expect.objectContaining({
'sentry.op': 'db',
'db.system.name': 'postgresql',
Expand Down Expand Up @@ -201,6 +202,7 @@ describe('postgresIntegration', () => {
expect(startInactiveSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
name: 'pg-pool.connect',
onlyIfParent: false,
attributes: expect.objectContaining({
'sentry.op': 'db',
'db.system.name': 'postgresql',
Expand Down
Loading