Repository: honeycombio/beeline-python (language: python)

Issue:
off-by-one error in `_should_sample`
**Versions**

- Python: any
- Beeline: any

**Steps to reproduce**

1. Set a `sample_rate` of 1 on an event, and choose a `trace_id` that (when hashed) starts 32 on bits, like `"acfe44e1069cc708fafc592b0f17b8c7"`
2. Call `_should_sample("acfe44e1069cc708fafc592b0f17b8c7", 1)`.

Expected return value: True. Actual return value: False.

**Additional context**

The problem is with the `<` instead of `<=` in `_should_sample`. This will not affect any other integral sample_rates, since  `sample_upper_bound` is always non-integral (MAX_INT32 is prime), _except_ if the sample rate is 1.

(Maybe for performance, the method could be prefixed with `if sample_rate == 1: return True`?)



Discussion / hints:

Fix patch:
diff --git a/beeline/trace.py b/beeline/trace.py
index edd37ea..7f5561e 100644
--- a/beeline/trace.py
+++ b/beeline/trace.py
@@ -373,6 +373,10 @@ def is_root(self):
 
 
 def _should_sample(trace_id, sample_rate):
+    # Always return True if sample rate is 1
+    if sample_rate == 1:
+        return True
+
     sample_upper_bound = MAX_INT32 / sample_rate
     # compute a sha1
     sha1 = hashlib.sha1()


Test patch:
diff --git a/beeline/test_trace.py b/beeline/test_trace.py
index 9bf9bab..43beece 100644
--- a/beeline/test_trace.py
+++ b/beeline/test_trace.py
@@ -5,6 +5,8 @@
 import json
 import unittest
 import re
+import os
+import binascii
 
 from libhoney import Event
 
@@ -121,6 +123,10 @@ def test_probability(self):
             self.assertLessEqual(sampled, acceptable_upper_bound)
             self.assertGreaterEqual(sampled, acceptable_lower_bound)
 
+    def should_sample_always_returns_true_when_sample_rate_is_1(self):
+        for _ in range(1000):
+            self.assertTrue(_should_sample(binascii.b2a_hex(os.urandom(16)), 1))
+
 
 class TestSynchronousTracer(unittest.TestCase):
     def test_trace_context_manager_exception(self):