-
Notifications
You must be signed in to change notification settings - Fork 5.4k
datadog: time_util portion of new tracing library #25003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2e32f68
4a02d36
fd44001
7f48705
dbb80b1
c8e308b
d253bfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| #include "source/extensions/tracers/datadog/time_util.h" | ||
|
|
||
| #include <chrono> | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Tracers { | ||
| namespace Datadog { | ||
|
|
||
| datadog::tracing::TimePoint estimateTime(SystemTime wall) { | ||
| return estimateTime(wall, datadog::tracing::default_clock); | ||
| } | ||
|
|
||
| datadog::tracing::TimePoint estimateTime(SystemTime wall, const datadog::tracing::Clock& clock) { | ||
| datadog::tracing::TimePoint point = clock(); | ||
| if (point.wall > wall) { | ||
| point.tick -= point.wall - wall; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also verify that the delta we are subtracting is less than point.tick?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, thanks! |
||
| } | ||
| point.wall = wall; | ||
| return point; | ||
| } | ||
|
|
||
| } // namespace Datadog | ||
| } // namespace Tracers | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| #pragma once | ||
|
|
||
| /** | ||
| * This file contains functions related to time points and durations. | ||
| * | ||
| * Envoy has a time type that contains both a system time point and a steady | ||
| * ("monotonic") time point. However, only the system time is exposed to the | ||
| * tracing subsystem. This may be remedied in the future, but for now we work | ||
| * with the system time. | ||
| * | ||
| * This is problematic for the Datadog core tracing library (dd-trace-cpp), | ||
| * because it uses the steady time to calculate the duration of a span | ||
| * (end.tick - begin.tick). So, we need to get a steady clock time from a given system | ||
| * clock time. The scheme is to measure the current system/steady time, | ||
| * compare the system part with the given system time, and then adjust the | ||
| * measured steady time accordingly. This is correct if the system clock has not | ||
| * been adjusted since the given system time was measured. It's incorrect | ||
| * otherwise, hence only an estimate. This conversion is performed by the | ||
| * estimateTime function. | ||
| */ | ||
|
|
||
| #include <datadog/clock.h> | ||
|
|
||
| #include "envoy/common/time.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Tracers { | ||
| namespace Datadog { | ||
|
|
||
| /** | ||
| * Convert a specified system \p time to a datadog time point, estimating the | ||
| steady portion of the result by examining the current time as measured by | ||
| the optionally specified \p clock and comparing it with the given \p time. | ||
| * @param time system time to convert from | ||
| * @param clock datadog clock used to measure the current time (default clock if omitted) | ||
| * @return datadog time point whose steady portion is estimated from the given \p time. | ||
| */ | ||
| datadog::tracing::TimePoint estimateTime(SystemTime time); | ||
| datadog::tracing::TimePoint estimateTime(SystemTime time, const datadog::tracing::Clock& clock); | ||
|
|
||
| } // namespace Datadog | ||
| } // namespace Tracers | ||
| } // namespace Extensions | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| #include <datadog/clock.h> | ||
|
|
||
| #include "envoy/common/time.h" | ||
|
|
||
| #include "source/extensions/tracers/datadog/time_util.h" | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Tracers { | ||
| namespace Datadog { | ||
| namespace { | ||
|
|
||
| TEST(DatadogTracerTimeUtilTest, EstimateTime) { | ||
| // Concerns: | ||
| // | ||
| // - If the current system time is after the specified time (likely case), | ||
| // then the resulting steady time point is set back accordingly. | ||
| // - If the current system time is before the specified time (rare case), | ||
| // then the resulting steady time point is whatever the clock returns. | ||
|
|
||
| // Modify `now` to change the value returned by `clock`. | ||
| datadog::tracing::TimePoint now; | ||
| datadog::tracing::Clock clock = [&]() { return now; }; | ||
|
|
||
| // A little time has elapsed since the SystemTime was measured. The | ||
| // resulting steady time point should be set back by the difference. | ||
| datadog::tracing::TimePoint clock_result = datadog::tracing::default_clock(); | ||
| SystemTime argument = clock_result.wall; | ||
| clock_result.wall += std::chrono::microseconds(100); | ||
| now = clock_result; | ||
| datadog::tracing::TimePoint result = estimateTime(argument, clock); | ||
| EXPECT_EQ(result.wall, argument); | ||
| EXPECT_EQ(result.tick, clock_result.tick - std::chrono::microseconds(100)); | ||
|
|
||
| // The clock has been set back since the SystemTime was measured. The | ||
| // resulting steady time can't do better than whatever the clock returns | ||
| // (we wouldn't want to set the steady time point into the future). | ||
| clock_result = datadog::tracing::default_clock(); | ||
| argument = clock_result.wall; | ||
| clock_result.wall -= std::chrono::milliseconds(100); | ||
| now = clock_result; | ||
| result = estimateTime(argument, clock); | ||
| EXPECT_EQ(result.wall, argument); | ||
| EXPECT_EQ(result.tick, clock_result.tick); | ||
| } | ||
|
|
||
| TEST(DatadogTracerTimeUtilTest, DefaultClock) { | ||
| // The one-parameter overload of `estimateTime` uses the system clock. | ||
| // We can at least check that the steady (monotonic, `.tick`) portion is | ||
| // approximately non-decreasing along "before," "during," and "after." | ||
| // Only "approximately," because the `datadog::tracing::default_clock` can't | ||
| // measure both clocks exactly simultaneously, so its correction to the | ||
| // steady time might actually set it back in time some tiny amount. | ||
| const datadog::tracing::Clock clock = datadog::tracing::default_clock; | ||
| const datadog::tracing::TimePoint before = clock(); | ||
| const datadog::tracing::TimePoint estimated_before = estimateTime(before.wall); | ||
| const datadog::tracing::TimePoint after = clock(); | ||
|
|
||
| const auto tolerance = std::chrono::microseconds(100); | ||
|
|
||
| EXPECT_LE(before.tick, after.tick); | ||
| EXPECT_LE(estimated_before.tick - before.tick, tolerance); | ||
| EXPECT_LE(after.tick - estimated_before.tick, tolerance); | ||
| } | ||
|
|
||
| } // namespace | ||
| } // namespace Datadog | ||
| } // namespace Tracers | ||
| } // namespace Extensions | ||
| } // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we not plumbing the MonotonicTime (or maybe just the TimeSource into here?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to implement the interfaces defined in https://github.com/envoyproxy/envoy/blob/main/envoy/tracing/trace_driver.h.
There are two time points in those interfaces: Span::spawnChild and Driver::startSpan. Both take
SystemTimeonly, without a monotonic counterpart.If I were redesigning the interface, I would have
spawnChildandstartSpantake both aSystemTimeand aMonotonicTime, and additionally havefinishSpantake aMonotonicTime(or both, but at least theMonotonicTime).I thought it would be inappropriate to propose to change the interface and everything that implements it.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to push forward with this but we should also try to correct the API to get the best results. It's not that hard to change these APIs; there's probably not that much fallout, particularly if you just plumb through some additional fields and don't remove the SystemTime.