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

feat(node): Add knex integration #13526

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Zen-cronic
Copy link
Contributor

@Zen-cronic Zen-cronic commented Aug 29, 2024

Resolves #13311

Implement Knex OTL instrumentation in packages/node.

This integration is not enabled by default.

Set up initial code and dependencies.
Enable integration by default.

Signed-off-by: Kaung Zin Hein <[email protected]>
@@ -47,6 +47,7 @@
"graphql": "^16.3.0",
"http-terminator": "^3.2.0",
"ioredis": "^5.4.1",
"knex": "^2.5.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change in major version 3 drops support for node < 16. See here.

Set up postgres for testing.
Update span attributes to be named under 'knex'.

Signed-off-by: Kaung Zin Hein <[email protected]>
@Zen-cronic Zen-cronic marked this pull request as ready for review August 29, 2024 17:17
Differentiate knex spans from other database spans.

Signed-off-by: Kaung Zin Hein <[email protected]>
const spanJSON = spanToJSON(span);
const spanData = spanJSON.data;

if (spanData && 'knex.version' in spanData) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In knex instr, the prop knex.version is added to the span attribute.

Add mysql2 client tests  using knex.
Add `knexIntegration` exports in several packages.

Signed-off-by: Kaung Zin Hein <[email protected]>
@Zen-cronic Zen-cronic changed the title feat(node): Add knexIntegration to Node feat(node): Add knex integration Aug 30, 2024
@Zen-cronic
Copy link
Contributor Author

Zen-cronic commented Aug 30, 2024

I've fixed all the failing CI builds/tests. The current fail is unrelated to the added integration.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Hey thanks for adding the instrumentation!

In general looks good to me. After we resolve the db.system concern we are good to merge.

If possible, could you share a screenshot of an example trace with knex instrumentation spans?


if (spanData && 'knex.version' in spanData) {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.knex');
span.setAttribute('db.system', 'knex');
Copy link
Member

@AbhiPrasad AbhiPrasad Sep 18, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted


export const instrumentKnex = generateInstrumentOnce(
INTEGRATION_NAME,
() => new KnexInstrumentation({ requireParentSpan: true }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, requireParentSpan is set to true by default. So, we'd have to expose the config. Similar discussion.


if (spanData && 'knex.version' in spanData) {
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.knex');
span.setAttribute('db.system', 'knex');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted

@AbhiPrasad AbhiPrasad self-requested a review November 7, 2024 15:23
@AbhiPrasad AbhiPrasad self-assigned this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add knexIntegration to Node
2 participants