datadog: time_util portion of new tracing library by dgoffredo · Pull Request #25003 · envoyproxy/envoy

dgoffredo · 2023-01-18T20:01:55Z

This is part of the initiative described in #23958.

This revision introduces a new library dependency for Datadog tracing, and adds a small component that will be used in the future when more of the new library is integrated into the Datadog tracing extension.

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

repokitteh-read-only · 2023-01-18T20:02:24Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #25003 was opened by dgoffredo.

see: more, trace.

dgoffredo · 2023-01-18T20:02:35Z

/assign @jmarantz

repokitteh-read-only · 2023-01-18T20:02:39Z

dgoffredo is not allowed to assign users.

🐱

Caused by: a #25003 (comment) was created by @dgoffredo.

see: more, trace.

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

jmarantz

lgtm mostly with some minor comments. still need to read the test.

jmarantz · 2023-01-19T14:32:05Z

+// specified `SystemTime`. Use the optionally specified
+// `datadog::tracing::Clock` to read the current time. If a clock is not
+// specified, then the default clock is used.
+datadog::tracing::TimePoint estimateTime(SystemTime);


is this an estimate really? or are we just converting a SystemTime to a DataDog time?

Would it be clearer at call-sites to call this convertSystemTimeToDataDog() ?

Yes, it's an estimate. The background is explained here as part of the original pull request (that readme will be added again in a subsequent pull request).

Envoy has a time type that, like Go's, contains both a system time point and a steady ("monotonic") time point. However, only the system time is exposed to the tracing subsystem. Not sure why.

This is problematic for the new 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 an estimate.

What do you think about all this? Is there a better way?

sgtm thanks. The above explanation might be good to throw into the code here as a comment.

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

jmarantz

looks pretty good. It'd be good to understand if we can just use MonotonicTime if that's what we need.

jmarantz · 2023-01-19T22:55:23Z

+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;


should we also verify that the delta we are subtracting is less than point.tick?

point.tick is a time point, while point.wall - wall is a duration, so no.

got it, thanks!

jmarantz · 2023-01-19T22:55:45Z

+namespace Tracers {
+namespace Datadog {
+
+datadog::tracing::TimePoint estimateTime(SystemTime wall) {


why are we not plumbing the MonotonicTime (or maybe just the TimeSource into here?

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 SystemTime only, without a monotonic counterpart.

If I were redesigning the interface, I would have spawnChild and startSpan take both a SystemTime and a MonotonicTime, and additionally have finishSpan take a MonotonicTime (or both, but at least the MonotonicTime).

I thought it would be inappropriate to propose to change the interface and everything that implements it.

What do you think?

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.

jmarantz

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2023-01-20T14:07:05Z

@envoyproxy/senior-maintainers assignee is @yanavlasov

🐱

Caused by: a #25003 (review) was submitted by @jmarantz.

see: more, trace.

wbpcode · 2023-01-23T03:16:53Z

/assign

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

yanavlasov · 2023-01-24T20:51:39Z

/retest

repokitteh-read-only · 2023-01-24T20:52:31Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #25003 (comment) was created by @yanavlasov.

see: more, trace.

dgoffredo · 2023-01-25T00:04:56Z

Code coverage for source/extensions/tracers is lower than limit of 95.4 (95.3)

dgoffredo · 2023-01-25T00:06:54Z

https://storage.googleapis.com/envoy-pr/3629d09/coverage/source/extensions/tracers/datadog/time_util.cc.gcov.html

I could write a test for that overload. Will do this tomorrow.

yanavlasov · 2023-01-25T15:48:31Z

/wait

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

yanavlasov · 2023-01-27T00:58:47Z

/retest

repokitteh-read-only · 2023-01-27T00:58:56Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #25003 (comment) was created by @yanavlasov.

see: more, trace.

dgoffredo · 2023-01-27T04:04:21Z

Thanks @yanavlasov @jmarantz

* datadog: time_util without tests Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

dgoffredo added 2 commits January 18, 2023 14:04

datadog: time_util without tests

2e32f68

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

datadog: time_util tests

4a02d36

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

dgoffredo requested a review from mattklein123 as a code owner January 18, 2023 20:01

repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Jan 18, 2023

repokitteh-read-only Bot assigned mattklein123 Jan 18, 2023

typo

fd44001

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

dgoffredo mentioned this pull request Jan 18, 2023

Datadog tracing extension: remove OpenTracing #23958

Closed

wrong year

7f48705

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

mattklein123 assigned jmarantz and unassigned mattklein123 Jan 18, 2023

jmarantz reviewed Jan 19, 2023

View reviewed changes

review comments: doxygen, auto, int diff comparison

dbb80b1

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

jmarantz reviewed Jan 19, 2023

View reviewed changes

jmarantz previously approved these changes Jan 20, 2023

View reviewed changes

repokitteh-read-only Bot assigned yanavlasov Jan 20, 2023

repokitteh-read-only Bot assigned wbpcode Jan 23, 2023

yanavlasov previously approved these changes Jan 23, 2023

View reviewed changes

yanavlasov enabled auto-merge (squash) January 23, 2023 15:34

datadog: background info for time_util

c8e308b

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

auto-merge was automatically disabled January 23, 2023 16:51
Head branch was pushed to by a user without write access

dgoffredo dismissed stale reviews from yanavlasov and jmarantz via c8e308b January 23, 2023 16:51

dgoffredo mentioned this pull request Jan 24, 2023

datadog: span portion of new tracing library #25121

Merged

repokitteh-read-only Bot added the waiting label Jan 25, 2023

datadog: test the one-parameter overload of estimateTime

d253bfb

Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

repokitteh-read-only Bot removed the waiting label Jan 25, 2023

yanavlasov approved these changes Jan 27, 2023

View reviewed changes

yanavlasov merged commit c476539 into envoyproxy:main Jan 27, 2023

ericvn mentioned this pull request Jan 27, 2023

Automator: update envoy@ in istio/proxy@master istio/proxy#4387

Closed

VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 2, 2023

datadog: time_util portion of new tracing library (envoyproxy#25003)

ff90f99

* datadog: time_util without tests Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

Conversation

dgoffredo commented Jan 18, 2023

Uh oh!

repokitteh-read-only Bot commented Jan 18, 2023

Uh oh!

dgoffredo commented Jan 18, 2023

Uh oh!

repokitteh-read-only Bot commented Jan 18, 2023

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgoffredo Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only Bot commented Jan 20, 2023

Uh oh!

wbpcode commented Jan 23, 2023

Uh oh!

yanavlasov commented Jan 24, 2023

Uh oh!

repokitteh-read-only Bot commented Jan 24, 2023

Uh oh!

dgoffredo commented Jan 25, 2023

Uh oh!

dgoffredo commented Jan 25, 2023

Uh oh!

yanavlasov commented Jan 25, 2023

Uh oh!

yanavlasov commented Jan 27, 2023

Uh oh!

repokitteh-read-only Bot commented Jan 27, 2023

Uh oh!

dgoffredo commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dgoffredo Jan 19, 2023 •

edited

Loading