Change span ID to use random bytes #654

draffensperger · 2018-01-18T15:55:56Z

While running a test application that was composed of multiple Node.js apps, each with tracing enabled, we found that span IDs would conflict when the multiple apps were simultaneously started, because each started their span IDs at index 1. This switches to use secure randomness for span IDs to help ensure uniqueness even when multiple Node apps are started at the same time.

src/span-data.ts

@@ -14,6 +14,7 @@
 * limitations under the License.
 */

+import {randomBytes} from 'crypto';


kjin · 2018-01-18T18:50:24Z

@draffensperger FYI: I opened a PR to update CONTRIBUTING.md (#655) to write about running unit tests locally. What was the problem you encountered with initializing test fixtures?

EDIT: For future readers -- the issue was that the last time npm run init-test-fixtures was run was before ea3b26e. To resolve this, delete any directories (but not the *.pem files) in test/plugins/fixtures

draffensperger · 2018-01-18T19:27:25Z

Thanks for documenting that!

As for my specific issue, I was getting an error like this:

[ { Error: ENOENT: no such file or directory, mkdir '~/github/cloud-trace-nodejs/build/test/plugins/fixtures/google-cloud-datastore1/node_modules/grpc/node_modules/tar-pack/test/fixtures'
   errno: -2,
   code: 'ENOENT',
   syscall: 'mkdir',
   path: '~/github/cloud-trace-nodejs/build/test/plugins/fixtures/google-cloud-datastore1/node_modules/grpc/node_modules/tar-pack/test/fixtures' } ]

It turns out that I still had some untracked directories in test/plugins/fixture from when I had cloned a previous version of the repo. I deleted those directories, but kept the .pem files and then I was able to run the tests. I also needed to run ./bin/docker-trace.sh start to start a Docker container with the databases for the test.

codecov · 2018-01-18T21:13:50Z

Codecov Report

Merging #654 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
+ Coverage   96.06%   96.08%   +0.01%     
==========================================
  Files          31       31              
  Lines        1627     1635       +8     
  Branches      293      295       +2     
==========================================
+ Hits         1563     1571       +8     
  Misses         64       64

Impacted Files	Coverage Δ
root/project/build/src/span-data.js	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b811387...4f5c977. Read the comment docs.

src/span-data.ts

@@ -14,6 +14,7 @@
 * limitations under the License.
 */

+import * as crypto from 'crypto';


test/test-trace-header-context.ts

+      var receivedSpanId = receivedSpanIdAndOptions[0];
+      var receivedTraceOptions = receivedSpanIdAndOptions[1];
+      // Trace ID and trace options should be the same in sender and receiver.
+      assert.equal(receivedTraceId, sentTraceId);


src/span-data.ts

-    () => randomFillSync(spanIdBuffer) :
-    () => randomBytes(SPAN_ID_RANDOM_BYTES);
+// This evaluates to a large whole number such that any fractional number
+// between 0 and 1 multiplied by it will always give a whole number.


src/span-data.ts

+
+function randomSpanId() {
+  // No rounding is needed because SPAN_ID_MAX is bigger than 1 / Number.EPSILON
+  return String(Math.random() * SPAN_ID_MAX);


kjin

LGTM. I'll merge this once CI passes

Change span ID to use random bytes

9c4a8f7

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 18, 2018

Add disable ban comment to fix tslint

259cb12

evanlucas reviewed Jan 18, 2018

View reviewed changes

src/span-data.ts Outdated

@@ -14,6 +14,7 @@

* limitations under the License.

*/

import {randomBytes} from 'crypto';

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

draffensperger added 2 commits January 18, 2018 11:05

Switch to use randomFillSync for performance

e221e5b

Use randomFillSync only if defined on crypto

1e6000d

draffensperger added 2 commits January 18, 2018 15:55

Fix tests and tidy up code

713f0ce

Remove destructuring to fix test for node 4

430f37e

Remove const/let from test cases

998f696

kjin self-requested a review January 18, 2018 21:32

kjin reviewed Jan 19, 2018

View reviewed changes

Use Math.random and strictEqual

6de97b7

kjin reviewed Jan 19, 2018

View reviewed changes

kjin mentioned this pull request Jan 19, 2018

chore: enable circleci cron and cover src/**/*.ts files only #651

Merged

Fix SPAN_ID_MAX to be clearer

a422a57

draffensperger force-pushed the random-span-ids branch from 859aa66 to a422a57 Compare January 19, 2018 18:45

kjin reviewed Jan 19, 2018

View reviewed changes

draffensperger added 3 commits January 19, 2018 15:50

Go back to using crypto random bytes

6556d73

Fix formatting

f5df082

Add back randomSpanId function I forgot

4f5c977

kjin approved these changes Jan 19, 2018

View reviewed changes

kjin merged commit a212d70 into googleapis:master Jan 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change span ID to use random bytes #654

Change span ID to use random bytes #654

draffensperger commented Jan 18, 2018 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

kjin commented Jan 18, 2018 •

edited

Loading

draffensperger commented Jan 18, 2018

codecov bot commented Jan 18, 2018 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

kjin left a comment

Change span ID to use random bytes #654

Change span ID to use random bytes #654

Conversation

draffensperger commented Jan 18, 2018 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

kjin commented Jan 18, 2018 • edited Loading

draffensperger commented Jan 18, 2018

codecov bot commented Jan 18, 2018 • edited Loading

Codecov Report

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

kjin left a comment

Choose a reason for hiding this comment

draffensperger commented Jan 18, 2018 •

edited

Loading

kjin commented Jan 18, 2018 •

edited

Loading

codecov bot commented Jan 18, 2018 •

edited

Loading