pass Datadog's internal shared tests for 128-bit trace IDs#41
Conversation
BenchmarksBenchmark execution time: 2023-07-06 22:33:12 Comparing candidate commit 7c1144c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
cgilmour
left a comment
cgilmour
left a comment
There was a problem hiding this comment.
Marking as approved.
Maybe some more checks and tests around zero-values will be needed for other system-tests.
Do all the related tests with valid propagation styles now pass? Any of them worth adding to this repo's unit tests?
| const auto seconds = | ||
| std::chrono::duration_cast<std::chrono::seconds>(since_epoch).count(); | ||
| // The farthest we'll go back is the unix epoch. | ||
| const std::uint64_t unsigned_seconds = seconds < 0 ? 0 : seconds; |
There was a problem hiding this comment.
It's not actually going to happen, but the lowest value should probably be 1, and not zero.
There was a problem hiding this comment.
I agree it's not going to happen. But why 1 and not 0?
My understanding of unix time is that it is the number of seconds elapsed since 00:00:00 UTC on 1 January 1970.
Hm, wait...
david@ein:~/Downloads$ python
Python 3.10.6 (main, May 29 2023, 11:10:38) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import datetime
>>> datetime.datetime.fromtimestamp(0)
datetime.datetime(1969, 12, 31, 19, 0)Ah, that's a time zone thing.
Anyway, the reason I have a cutoff here is to prevent the rollover to 18446744073709551615 at -1 (which is not a valid unix timestamp, but would correspond to a date over 584 trillion years in the future -- build software to last!).
| const auto high = parse_uint64(found->second, 16); | ||
| REQUIRE(high); | ||
| REQUIRE(*high == generated_id.high); | ||
| const std::uint64_t expected = std::uint64_t(flash_crash) << 32; |
There was a problem hiding this comment.
I'd probably compare this with a raw expected value instead of a computed one.
There was a problem hiding this comment.
That's a fine line and I see your point. The test is kind of circular if it just does what the implementation does.
But if I were to include the raw expected value in the test, I'd obtain it by calculating 1273171513 << 32 (which is 5468230010533838848). At that point we'd be testing that operator<<(uint64_t, int) behaves the same way for all build targets, which is meh.
|
Thanks for the review, Caleb.
Maybe so. I'm starting those today.
All tests in
I added unit tests wherever the library's behavior departed from that expected by the shared tests. The consistent behavior is already covered by existing unit tests. |
- Add uWSGI service for testing and as an example - Add propagation tests for uwsgi directives - Update example/README.md - Ignore *.pyc - Mention uwsgi_pass in the docs for datadog_proxy_directive Resolves DataDog#38
2 participants
These are the changes necessary for dd-trace-cpp to pass applicable test cases in
tests/parametric/test_128_bit_traceids.pyin Datadog's internal shared tests.See the commit messages for the correspondence with shared tests.