fix(mysql): bind get connection callback to active context by sstone1 · Pull Request #562 · open-telemetry/opentelemetry-js-contrib

sstone1 · 2021-07-05T15:27:35Z

Signed-off-by: Simon Stone simonstone1@gmail.com

Which problem is this PR solving?

The problem described in issue MySQL spans do not link to the active parent span #561

Short description of the changes

The callback for getConnection is now bound to the active context before it is passed into the instrumented mysql code

linux-foundation-easycla · 2021-07-05T15:27:38Z

The committers are authorized under a signed CLA.

✅ Simon Stone (bc37bf7)

plugins/node/opentelemetry-instrumentation-mysql/src/instrumentation.ts

codecov · 2021-07-05T17:50:13Z

Codecov Report

Merging #562 (9d254e4) into main (72b78e6) will decrease coverage by 1.64%.
The diff coverage is n/a.

❗ Current head 9d254e4 differs from pull request most recent head 54b3191. Consider uploading reports for the commit 54b3191 to get more accurate results

@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
- Coverage   96.68%   95.04%   -1.65%     
==========================================
  Files          13      204     +191     
  Lines         634    12003   +11369     
  Branches      124     1137    +1013     
==========================================
+ Hits          613    11408   +10795     
- Misses         21      595     +574

Impacted Files	Coverage Δ
...metry-propagator-ot-trace/src/OTTracePropagator.ts	`95.83% <0.00%> (ø)`
...er-extension-autoinjection/src/background/index.ts	`0.00% <0.00%> (ø)`
.../opentelemetry-instrumentation-router/src/utils.ts	`100.00% <0.00%> (ø)`
...ource-detector-aws/src/detectors/AwsEc2Detector.ts	`97.91% <0.00%> (ø)`
...pentelemetry-instrumentation-router/src/version.ts	`100.00% <0.00%> (ø)`
...opentelemetry-instrumentation-graphql/src/utils.ts	`95.26% <0.00%> (ø)`
packages/opentelemetry-host-metrics/src/enum.ts	`100.00% <0.00%> (ø)`
...metry-resource-detector-aws/src/detectors/index.ts	`100.00% <0.00%> (ø)`
...umentation-cassandra/test/cassandra-driver.test.ts	`95.77% <0.00%> (ø)`
...etry-instrumentation-hapi/test/hapi-plugin.test.ts	`100.00% <0.00%> (ø)`
... and 181 more

obecny

one comment

plugins/node/opentelemetry-instrumentation-mysql/src/instrumentation.ts

obecny

lgtm

vmarchaud

requesting changes so we don't merge it before addresing flarna feedback

blumamir · 2021-07-13T05:53:25Z

plugins/node/opentelemetry-instrumentation-mysql/test/index.test.ts

+          assert.ifError(err);
+          assert.ok(connection);
+          const sql = 'SELECT ? as solution';
+          connection.query(sql, 1, (err, res) => {
+            assert.ifError(err);
+            assert.ok(res);
+            assert.strictEqual(res[0].solution, 1);
+            const spans = memoryExporter.getFinishedSpans();
+            assert.strictEqual(spans.length, 1);
+            const querySpan = spans[0];
+            assert.strictEqual(
+              querySpan.parentSpanId,
+              parentSpan.spanContext().spanId
+            );
+            done();
+          });
+        });


This is a very verbose way to check that the context is correctly set in the callback.
Alternative and more direct option is to extract trace.getSpan(context.active()) in the callback and compare it to parentSpan

@blumamir thanks, I've changed the test.

Signed-off-by: Simon Stone <simonstone1@gmail.com>

Signed-off-by: Simon Stone <Simon.Stone@docusign.com>

.

Signed-off-by: Simon Stone <Simon.Stone@docusign.com>

sstone1 requested a review from a team July 5, 2021 15:27

sstone1 force-pushed the issue-561 branch 2 times, most recently from bed4685 to bc37bf7 Compare July 5, 2021 15:35

vmarchaud reviewed Jul 5, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-mysql/src/instrumentation.ts Outdated Show resolved Hide resolved

sstone1 force-pushed the issue-561 branch 2 times, most recently from 844781f to 3454156 Compare July 5, 2021 18:18

sstone1 requested a review from vmarchaud July 5, 2021 18:19

vmarchaud approved these changes Jul 5, 2021

View reviewed changes

vmarchaud requested a review from a team July 5, 2021 18:35

obecny reviewed Jul 5, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-mysql/src/instrumentation.ts Outdated Show resolved Hide resolved

sstone1 force-pushed the issue-561 branch from 3454156 to 3ebf81d Compare July 6, 2021 10:24

sstone1 requested a review from obecny July 6, 2021 10:25

vmarchaud requested a review from a team July 12, 2021 08:45

Flarna reviewed Jul 12, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-mysql/src/instrumentation.ts Outdated Show resolved Hide resolved

obecny approved these changes Jul 12, 2021

View reviewed changes

MSNev approved these changes Jul 12, 2021

View reviewed changes

vmarchaud previously requested changes Jul 12, 2021

View reviewed changes

blumamir reviewed Jul 13, 2021

View reviewed changes

sstone1 and others added 2 commits August 10, 2021 11:05

fix(mysql): bind get connection callback to active context

25b93b1

Signed-off-by: Simon Stone <simonstone1@gmail.com>

fix(mysql): address review comments from @Flarna, @blumamir

5f192b8

Signed-off-by: Simon Stone <Simon.Stone@docusign.com>

sstone1 force-pushed the issue-561 branch from 1540bd0 to 5f192b8 Compare August 11, 2021 08:21

sstone1 requested a review from vmarchaud August 11, 2021 08:26

vmarchaud requested review from Flarna and blumamir August 11, 2021 08:57

fix(mysql): fix linting errors

0c73381

Signed-off-by: Simon Stone <Simon.Stone@docusign.com>

Flarna approved these changes Aug 13, 2021

View reviewed changes

vmarchaud approved these changes Aug 14, 2021

View reviewed changes

Merge branch 'main' into issue-561

54b3191

vmarchaud merged commit 90d4e78 into open-telemetry:main Aug 14, 2021

vmarchaud added the bug Something isn't working label Aug 14, 2021

vmarchaud linked an issue Aug 14, 2021 that may be closed by this pull request

MySQL spans do not link to the active parent span #561

Closed

dyladan mentioned this pull request Feb 28, 2022

chore: release main #924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mysql): bind get connection callback to active context#562

fix(mysql): bind get connection callback to active context#562
vmarchaud merged 4 commits intoopen-telemetry:mainfrom
sstone1:issue-561

sstone1 commented Jul 5, 2021 •

edited

Loading

Uh oh!

linux-foundation-easycla bot commented Jul 5, 2021 •

edited

Loading

Uh oh!

Uh oh!

codecov bot commented Jul 5, 2021 •

edited

Loading

Uh oh!

obecny left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

obecny left a comment

Uh oh!

vmarchaud left a comment

Uh oh!

blumamir Jul 13, 2021

Uh oh!

sstone1 Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

sstone1 commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Short description of the changes

Uh oh!

linux-foundation-easycla bot commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

obecny left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

obecny left a comment

Choose a reason for hiding this comment

Uh oh!

vmarchaud left a comment

Choose a reason for hiding this comment

Uh oh!

blumamir Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

sstone1 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sstone1 commented Jul 5, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 5, 2021 •

edited

Loading

codecov bot commented Jul 5, 2021 •

edited

Loading

obecny left a comment •

edited

Loading