Speed up id generation

[bplotnick]

For traces with lots of spans, generating span ids can add up. This branch changes generate_random_{64,128}bit_string from using os.urandom to using random.getrandbits, which is faster. It also adds benchmarking which shows a speedup of over 2x. The boolean is whether to use 128bit ids. I'm also not sure why the max is so high, maybe a gc pause?:

old:
------------------------------------------------------------------------- benchmark 'test_create_attrs_for_span': 2 tests -------------------------------------------------------------------------
Name (time in us)                   Min                 Max              Mean            StdDev            Median               IQR            Outliers  OPS (Kops/s)            Rounds  Iterations
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
create_attrs_for_span[False]     7.7169 (1.0)      198.1538 (1.12)     9.0767 (1.0)      6.3648 (2.03)     8.1807 (1.0)      0.2161 (1.33)       54;113      110.1723 (1.0)        1423           1
create_attrs_for_span[True]      8.5458 (1.11)     177.1487 (1.0)      9.3490 (1.03)     3.1416 (1.0)      9.0655 (1.11)     0.1621 (1.0)      473;1897      106.9628 (0.97)      36254           1
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

new:
------------------------------------------------------------------------- benchmark 'test_create_attrs_for_span': 2 tests -------------------------------------------------------------------------
Name (time in us)                   Min                 Max              Mean            StdDev            Median               IQR            Outliers  OPS (Kops/s)            Rounds  Iterations
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
create_attrs_for_span[False]     2.7791 (1.0)      158.6787 (1.0)      3.3887 (1.0)      1.8108 (1.08)     3.2373 (1.0)      0.1006 (1.08)     406;1377      295.0969 (1.0)       23452           1
create_attrs_for_span[True]      3.0342 (1.09)     163.6483 (1.03)     3.5856 (1.06)     1.6776 (1.0)      3.4552 (1.07)     0.0931 (1.0)     1024;4203      278.8915 (0.95)      73384           1
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

[coveralls]

Coverage remained the same at 100.0% when pulling b06d25d on bplotnick:make-random-fast-again into 70c58b1 on Yelp:master.

[thebostik]

lgtm!

[kaisen]

Nice results! Are we confident that the generated numbers have enough entropy? Might be worth running a quick test to see if any duplicate ids are generated in 10000 runs?

[kaisen]

After reading through the docs for getrandbits, this branch lg2m. Feel free to ignore my comments :)

[codefromthecrypt]

Not sure it will help with performance, but having a byte prefix of epoch seconds allows transparent conversion to AWS IDs and.. natural sort/partitioning by time.

openzipkin/b3-propagation#6

Here's an example implementation in java

  static long nextTraceIdHigh(Random prng) {
    long epochSeconds = System.currentTimeMillis() / 1000;
    int random = prng.nextInt();
    return (epochSeconds & 0xffffffffL) << 32
        |  (random & 0xffffffffL);
  }

[codefromthecrypt]

most trace ID sampling only needs the lower 64-bits to be precisely random, and collision aversion should be pretty good, too, at least as good as AWS.

[coveralls]

Coverage remained the same at 100.0% when pulling 9d7a4eb on bplotnick:make-random-fast-again into 70c58b1 on Yelp:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 9d7a4eb on bplotnick:make-random-fast-again into 70c58b1 on Yelp:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 9d7a4eb on bplotnick:make-random-fast-again into 70c58b1 on Yelp:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 9d7a4eb on bplotnick:make-random-fast-again into 70c58b1 on Yelp:master.

[bplotnick]

I've gone ahead and changed the 128-bit generator to set the most significant 32 bits as epoch seconds and the other 96 bits as random, so that we get x-ray interop.

This adds about 30% overhead (almost a microsecond on my machine), however this is probably not a big deal given that traceids are only generated once per trace.

There is also a small query performance concern with the epoch128 format. We sometimes do sampling by prefix by doing something like traceId=b*, which gives a 1/16 sampling. We could do a suffix query (e.g. traceId=*b), but this is apparently less efficient for some systems.

In practice, this is not a huge deal since you can always pass in your own traceids (we do this using pyramid_zipkin), but it's something to think about.

[codefromthecrypt]

Ah interesting about prefix sampling.. if your sampling is offset would it be worse? Ex would it cost a lot to do "prefix" but offset 8 chars?

…

On 12 Jan 2018 5:09 am, "Ben Plotnick" ***@***.***> wrote: I've gone ahead and changed the 128-bit generator to set the most significant 32 bits as epoch seconds and the other 96 bits as random, so that we get x-ray interop. This adds about 30% overhead (almost a microsecond on my machine), however this is probably not a big deal given that traceids are only generated once per trace. There is also a small query performance concern with the epoch128 format. We sometimes do sampling by prefix by doing something like traceId=b*, which gives a 1/16 sampling. We could do a suffix query (e.g. traceId=*b), but this is apparently less efficient for some systems. In practice, this is not a huge deal since you can always pass in your own traceids (we do this using pyramid_zipkin <http://pyramid-zipkin.readthedocs.io/en/latest/configuring_zipkin.html#zipkin-trace-id-generator>), but it's something to think about. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61_EfeA4nNFks1PRsnifzYqO9nT1Cks5tJnhwgaJpZM4RaMvL> .

[bplotnick]

@adriancole I'm not sure about that. For splunk (which we use heavily), it generally says that wildcards in the beginning or middle can have performance issues https://docs.splunk.com/Documentation/SplunkCloud/6.6.3/Search/Wildcards#Prefix_wildcards_might_cause_performance_issues. I imagine that a regex-based search might be efficient in this manner. @thebostik any idea on this?

[kaisen]

[codefromthecrypt]

Yeah was thinking fixed offset not wildcard but regardless it is probably too niche anyway.. interesting to know impact though so i can summarize on the related issue.

…

On 13 Jan 2018 3:09 am, "Ben Plotnick" ***@***.***> wrote: @adriancole <https://github.com/adriancole> I'm not sure about that. For splunk (which we use heavily), it generally says that wildcards in the beginning or middle can have performance issues https://docs.splunk.com/ Documentation/SplunkCloud/6.6.3/Search/Wildcards#Prefix_ wildcards_might_cause_performance_issues. I imagine that a regex-based search might be efficient in this manner. @thebostik <https://github.com/thebostik> any idea on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD6191JyfQEwVinLTM5v8SbG5TSUlvEks5tJ6uJgaJpZM4RaMvL> .