[DSM] Guard against future timestamps when extracting pathway context#8672
Conversation
|
vandonr
left a comment
There was a problem hiding this comment.
approving with minor comments
| /// Tolerance (in nanoseconds) applied when validating that decoded pathway/edge timestamps | ||
| /// are not in the future. Allows for normal clock drift between hosts. | ||
| /// </summary> | ||
| internal const long MaxClockSkewNs = 60L * 1_000_000_000L; |
There was a problem hiding this comment.
yeah idk I'd have used something more loose like 5 minutes, but really this is wet finger engineering
There was a problem hiding this comment.
Yeah, exactly, I'm fine with 5 minutes too 🤷♂️ I don't know if it's something we have actually observed in practice, so we probably don't have any data to go on either 🤷♂️
There was a problem hiding this comment.
if only we had observability ![]()
BenchmarksBenchmark execution time: 2026-05-21 15:37:36 Comparing candidate commit a8e05f0 in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 8 performance improvements and 12 performance regressions! Performance is the same for 45 metrics, 7 unstable metrics, 92 known flaky benchmarks, 34 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8672) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (74ms) : 70, 77
master - mean (73ms) : 70, 76
section Bailout
This PR (8672) - mean (77ms) : 75, 80
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8672) - mean (1,116ms) : 1043, 1190
master - mean (1,117ms) : 1055, 1179
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (114ms) : 109, 119
master - mean (114ms) : 109, 119
section Bailout
This PR (8672) - mean (117ms) : 111, 123
master - mean (118ms) : 113, 124
section CallTarget+Inlining+NGEN
This PR (8672) - mean (788ms) : 767, 810
master - mean (787ms) : 764, 810
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (106ms) : 101, 110
master - mean (105ms) : 100, 110
section Bailout
This PR (8672) - mean (105ms) : 99, 111
master - mean (103ms) : 100, 105
section CallTarget+Inlining+NGEN
This PR (8672) - mean (952ms) : 914, 990
master - mean (945ms) : 909, 982
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (100ms) : 97, 102
master - mean (100ms) : 95, 105
section Bailout
This PR (8672) - mean (104ms) : 99, 109
master - mean (104ms) : 99, 108
section CallTarget+Inlining+NGEN
This PR (8672) - mean (829ms) : 793, 866
master - mean (821ms) : 787, 854
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (203ms) : 199, 206
master - mean (202ms) : 197, 207
section Bailout
This PR (8672) - mean (206ms) : 201, 210
master - mean (206ms) : 202, 209
section CallTarget+Inlining+NGEN
This PR (8672) - mean (1,210ms) : 1157, 1263
master - mean (1,206ms) : 1160, 1253
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (291ms) : 284, 298
master - mean (291ms) : 284, 298
section Bailout
This PR (8672) - mean (291ms) : 283, 299
master - mean (291ms) : 284, 297
section CallTarget+Inlining+NGEN
This PR (8672) - mean (972ms) : 953, 992
master - mean (969ms) : 950, 988
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (282ms) : 276, 288
master - mean (283ms) : 277, 290
section Bailout
This PR (8672) - mean (282ms) : 276, 289
master - mean (283ms) : 277, 290
section CallTarget+Inlining+NGEN
This PR (8672) - mean (1,166ms) : 1125, 1207
master - mean (1,164ms) : 1119, 1208
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8672) - mean (280ms) : 274, 286
master - mean (281ms) : 274, 287
section Bailout
This PR (8672) - mean (282ms) : 276, 288
master - mean (282ms) : 276, 288
section CallTarget+Inlining+NGEN
This PR (8672) - mean (1,045ms) : 1001, 1089
master - mean (1,040ms) : 997, 1083
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary of changes
Guards against future timestamps when extracting pathwayContext and calculating
pathwayLatencyNsReason for change
We decode the
dd-pathway-ctxfrom inbound headers, and use the decodedPathwayStartto computepathwayLatencyNs. This is then used to calculate anoriginTimestampand is added to_originBuckets. We then only flush_originBucketsfor which the start time is <= the current bucket window.If a "future" timestamp ends up in
PathwayStart, then we would end up with that future bucket never being flushed and accumulating. This PR guards against those future timestamps so we don't end up with unbounded growth.Implementation details
Add two guards:
SetCheckpoint, guard against generating negativepathwayLatencyNsoredgeLatencyNsPathwayContextEncoder.Decode, reject data points with a "future"PathwayStarttimeTest coverage
Added a bunch of additional unit tests around this.