-
Notifications
You must be signed in to change notification settings - Fork 549
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
feat(instrumentation-pg): add post-query custom attributes hook for spans #767
Changes from all commits
d6f8af0
85d5611
27a4050
07924d6
f724864
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -563,5 +563,113 @@ describe('pg', () => { | |
client.query('SELECT NOW()').then(queryHandler); | ||
}); | ||
}); | ||
|
||
it('should call postQueryHook with query text if set', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can wrap this section with a |
||
instrumentation.disable(); | ||
let called = false; | ||
const query = 'SELECT NOW()'; | ||
const config: PgInstrumentationConfig = { | ||
postQueryHook: ctx => { | ||
called = true; | ||
assert.strictEqual(ctx.query, query); | ||
assert.strictEqual(ctx.params, undefined); | ||
Comment on lines
+574
to
+575
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These assertions are performed in the hook body. |
||
}, | ||
}; | ||
instrumentation.setConfig(config); | ||
instrumentation.enable(); | ||
|
||
const attributes = { | ||
...DEFAULT_ATTRIBUTES, | ||
[SemanticAttributes.DB_STATEMENT]: query, | ||
}; | ||
const events: TimedEvent[] = []; | ||
const span = tracer.startSpan('test span'); | ||
await context.with(trace.setSpan(context.active(), span), async () => { | ||
try { | ||
const resPromise = await client.query(query); | ||
assert.ok(resPromise); | ||
runCallbackTest(span, attributes, events); | ||
} catch (e) { | ||
assert.ok(false, e.message); | ||
} | ||
}); | ||
assert.strictEqual(called, true); | ||
}); | ||
|
||
it('should call postQueryHook with query text and params if set', async () => { | ||
instrumentation.disable(); | ||
let called = false; | ||
const values = ['0']; | ||
const query = 'SELECT $1::text'; | ||
const config: PgInstrumentationConfig = { | ||
postQueryHook: ctx => { | ||
called = true; | ||
assert.strictEqual(ctx.query, query); | ||
assert.strictEqual(ctx.params, values); | ||
}, | ||
}; | ||
instrumentation.setConfig(config); | ||
instrumentation.enable(); | ||
|
||
const attributes = { | ||
...DEFAULT_ATTRIBUTES, | ||
[SemanticAttributes.DB_STATEMENT]: query, | ||
}; | ||
const events: TimedEvent[] = []; | ||
const span = tracer.startSpan('test span'); | ||
await context.with(trace.setSpan(context.active(), span), async () => { | ||
const resPromise = await client.query(query, values); | ||
try { | ||
assert.ok(resPromise); | ||
runCallbackTest(span, attributes, events); | ||
} catch (e) { | ||
assert.ok(false, e.message); | ||
} | ||
}); | ||
assert.strictEqual(called, true); | ||
}); | ||
|
||
it('should call postQueryHook with query config if set', async () => { | ||
instrumentation.disable(); | ||
const name = 'fetch-text'; | ||
const query = 'SELECT $1::text'; | ||
const values = ['0']; | ||
let called = false; | ||
const config: PgInstrumentationConfig = { | ||
postQueryHook: ctx => { | ||
called = true; | ||
if (!ctx.config) { | ||
assert.ok(false, 'ctx.config was undefined'); | ||
} | ||
assert.strictEqual(ctx.config.text, query); | ||
assert.strictEqual(ctx.config.values, values); | ||
}, | ||
}; | ||
instrumentation.setConfig(config); | ||
instrumentation.enable(); | ||
|
||
const attributes = { | ||
...DEFAULT_ATTRIBUTES, | ||
[AttributeNames.PG_PLAN]: name, | ||
[SemanticAttributes.DB_STATEMENT]: query, | ||
}; | ||
const events: TimedEvent[] = []; | ||
const span = tracer.startSpan('test span'); | ||
|
||
await context.with(trace.setSpan(context.active(), span), async () => { | ||
try { | ||
const resPromise = await client.query({ | ||
name: name, | ||
text: query, | ||
values: values, | ||
}); | ||
assert.strictEqual(resPromise.command, 'SELECT'); | ||
runCallbackTest(span, attributes, events); | ||
} catch (e) { | ||
assert.ok(false, e.message); | ||
} | ||
}); | ||
assert.strictEqual(called, true); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also suggest having some negative tests for the hook - what happens if it throws? what if it's not a function? |
||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change the hook signature to follow the other hook example in this file:
First parameter should be the span.
Second parameter is called
Info
on the other hook. Could be nice in my opinion to call itqueryInfo
(to stay consistent with the existing name.From what I remember, this is also the convention in other instrumentations.