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

dgoffredo · 2023-03-17T18:47:28Z

This is part of the work proposed in #23958.

This adds the Tracer class, which has one member function, startSpan, for creating a new trace or extracting one from incoming (HTTP) request headers.

Tracer is a thread-local wrapper around the corresponding class in dd-trace-cpp, except that failure to configure the underlying tracer puts Tracer into a "no-op mode" that always produces NullSpan instances.

In addition to that code and its corresponding unit tests, I also updated the version of dd-trace-cpp used. The newer version has additional Span methods that made testing easier. Additionally, the newer dd-trace-cpp models TraceID as its own class to accommodate 128-bit IDs. The existing unit test for Span needed to be modified to account for this.

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

….goffredo/dd-trace-cpp-3/9-tracer_stats' of github.com:DataDog/envoy into david.goffredo/dd-trace-cpp-7/9-agent_http_client Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

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

…lient

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

repokitteh-read-only · 2023-03-17T18:47:37Z

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: #26148 was opened by dgoffredo.

see: more, trace.

jmarantz · 2023-03-23T21:10:27Z

many apologies for the delay in responsiveness on this and other PRs; I will try to catch up over the weekend.

dgoffredo · 2023-03-24T15:15:30Z

No problem, thanks for taking a look.

After this one there's the final PR that I proposed yesterday. Almost there!

jmarantz

Looks great at a high level.

I feel like we are getting into Datadog semantics which I am not familiar with; would it make sense to also get a review from someone else familiar with this tracing infrastructure?

/wait

jmarantz · 2023-03-25T12:21:32Z

+  config.agent.http_client = std::make_shared<AgentHTTPClient>(
+      cluster_manager, collector_cluster, collector_reference_host, tracer_stats);
+
+  auto maybe_config = datadog::tracing::finalize_config(config);


can you give an explicit type here? Not obvious from context. I'm assuming something similar to absl::StatusOr.

jmarantz · 2023-03-25T12:23:19Z

+
+  auto maybe_config = datadog::tracing::finalize_config(config);
+  if (auto* error = maybe_config.if_error()) {
+    auto prefix = "Unable to configure Datadog tracer. Tracing is now disabled. Error: ";


I've never seen this pattern for declaring string constants. Maybe you could explicitly use whatever type with_prefix expects to avoid potentially implicitly converting between them?

Not that it matters for performance obviously.

jmarantz · 2023-03-25T12:31:50Z

+Tracing::SpanPtr Tracer::startSpan(const Tracing::Config&, Tracing::TraceContext& trace_context,
+                                   const std::string& operation_name, SystemTime start_time,
+                                   const Tracing::Decision tracing_decision) {
+  auto& thread_local_tracer = thread_local_slot_->getTyped<ThreadLocalTracer>();


consider using TypedSlot for thread-safe slots.

IMO the untyped slots are tech-debt; it would be good to remove them from the public API for TheadLocalStore, but it's so hard to find the time for things like this :)

here's an example usage: source/common/rds/rds_route_config_provider_impl.h:46

jmarantz · 2023-03-25T12:32:24Z

+
+  datadog::tracing::Tracer& tracer = *thread_local_tracer.tracer;
+  TraceContextReader reader{trace_context};
+  auto maybe_span = tracer.extract_span(reader, span_config);


nit: explicit type

jmarantz · 2023-03-25T12:32:48Z

+    maybe_span = tracer.create_span(span_config);
+  }
+
+  assert(maybe_span);


ASSERT or RELEASE_ASSERT per Envoy style?

RELEASE_ASSERT would be more like assert() (can abort production binaries)

jmarantz · 2023-03-25T12:34:53Z

+class Tracer : public Tracing::Driver, private Logger::Loggable<Logger::Id::tracing> {
+public:
+  /**
+   * Create a \c Tracer that produces traces according to the specified \p config


what do these back-slash escaped sequences do?

Do they render well in some IDE? I have not noticed these used in Envoy.

This is doxygen-style fixed-width formatting. I think that \c means "class" and \p means "parameter," though the doxygen docs point out that they can be used interchangeably.

My editor changes the color, and presumably rendered doxygen puts the following word in <code></code> tags.

Envoy code I've looked at doesn't use these operators. I started using them as a substitute for backticks when in an earlier review you advised me to comment doxygen-style.

I could remove them, but I like them. Up to you.

jmarantz · 2023-03-25T12:40:06Z

+  Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(),
+                thread_local_slot_allocator_);
+
+  (void)tracer;


is this needed to avoid a compile error?

Envoy style is to use UNREFERENCED_PARAMETER from ./source/common/common/macros.h

dgoffredo · 2023-03-27T21:10:39Z

@cgilmour Please take a look at this with Datadog eyes.

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

cgilmour · 2023-03-30T09:51:57Z

For the datadog-specific bits, I'm happy to confirm about this.
The intention with the earlier (opentracing-based) integration and with this one was to be both comprehensive but also low-footprint, so that future changes are "just a version-bump".

We both are employed by Datadog, and appreciate being a feature in this project.

dgoffredo · 2023-03-30T17:14:50Z

Thanks, Caleb.

@jmarantz I addressed your earlier comments in a recent commit (except for the part about doxygen -- let me know).

Once we're good on this PR, I'll massage it into #26284 and that'll be the home stretch.

jmarantz

Thanks!

jmarantz · 2023-03-30T17:19:21Z

/retest

repokitteh-read-only · 2023-03-30T17:19:26Z

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

🐱

Caused by: a #26148 (comment) was created by @jmarantz.

see: more, trace.

This is part of the work proposed in envoyproxy#23958. This adds the Tracer class, which has one member function, startSpan, for creating a new trace or extracting one from incoming (HTTP) request headers. Tracer is a thread-local wrapper around the corresponding class in dd-trace-cpp, except that failure to configure the underlying tracer puts Tracer into a "no-op mode" that always produces NullSpan instances. In addition to that code and its corresponding unit tests, I also updated the version of dd-trace-cpp used. The newer version has additional Span methods that made testing easier. Additionally, the newer dd-trace-cpp models TraceID as its own class to accommodate 128-bit IDs. The existing unit test for Span needed to be modified to account for this. Signed-off-by: David Goffredo <david.goffredo@datadoghq.com> Signed-off-by: River Phillips <riverphillips1@gmail.com>

This is the final part of the work proposed in #23958. This PR additionally contains all of the changes from #26148. This diff will shrink when that PR is merged. This revision alters the DatadogTracerFactory in source/extensions/tracers/datadog/config.h to produce instances of the new dd-trace-cpp based implementation of the Datadog tracer, instead of the old dd-opentracing-cpp based one. This revision actually alters the implementation of the Datadog tracer. Previous pull requests have only added code for use here. This revision also moves some unit tests around: Two tests that were part of datadog_tracer_impl_test.cc have been moved into agent_http_client_test.cc, because they're about HTTP requests. One test that was part of datadog_tracer_impl_test.cc has been removed because its behavior is already covered by tests added in other PRs in this series. The remaining tests in datadog_tracer_impl_test_.cc were moved into config_test.cc, since they all now have to do with the configuration of the tracer and the resulting behavior. Finally, all references to dd-opentracing-cpp and msgpack have been removed from the repo, and the documentation (READMEs) and demo/ directory from #23958 have been added. Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

) This is the final part of the work proposed in envoyproxy#23958. This PR additionally contains all of the changes from envoyproxy#26148. This diff will shrink when that PR is merged. This revision alters the DatadogTracerFactory in source/extensions/tracers/datadog/config.h to produce instances of the new dd-trace-cpp based implementation of the Datadog tracer, instead of the old dd-opentracing-cpp based one. This revision actually alters the implementation of the Datadog tracer. Previous pull requests have only added code for use here. This revision also moves some unit tests around: Two tests that were part of datadog_tracer_impl_test.cc have been moved into agent_http_client_test.cc, because they're about HTTP requests. One test that was part of datadog_tracer_impl_test.cc has been removed because its behavior is already covered by tests added in other PRs in this series. The remaining tests in datadog_tracer_impl_test_.cc were moved into config_test.cc, since they all now have to do with the configuration of the tracer and the resulting behavior. Finally, all references to dd-opentracing-cpp and msgpack have been removed from the repo, and the documentation (READMEs) and demo/ directory from envoyproxy#23958 have been added. Signed-off-by: David Goffredo <david.goffredo@datadoghq.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

dgoffredo added 30 commits January 18, 2023 15:09

dd-trace-cpp source/ dependency

12ca34d

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

dd-trace-cpp test/ dependency

5408e3c

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

wrong year

d0332e4

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

TODO: AMEND

d44b37d

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

datadog: dict_util with tests

68cb723

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

datadog: add tracer_stats without tests

63ca86f

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

datadog: add tracer_stats tests

63f5f68

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

datadog: add comment describing dict_util

2555875

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

datadog: move private data members to bottom of class definition

9da7087

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

datadog: add class EventScheduler, untested

ef42164

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

datadog: test EventScheduler

246bf0e

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

ci/run_envoy_docker.sh './ci/do_ci.sh format'

55118c8

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

Merge branches 'david.goffredo/dd-trace-cpp-2/9-dict_util' and 'david…

efb9761

….goffredo/dd-trace-cpp-3/9-tracer_stats' of github.com:DataDog/envoy into david.goffredo/dd-trace-cpp-7/9-agent_http_client Signed-off-by: David Goffredo <david.goffredo@datadoghq.com>

datadog: AgentHttpClient without tests

0c930aa

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

Merge branch 'main' into david.goffredo/dd-trace-cpp-6/9-event_scheduler

9f28856

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

datadog: AgentHttpClient dummy test and some documentation

d9e989d

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

merge "main" and change some #include styles

5dee2fe

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

document AgentHTTPClient

7c019cf

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

datadog: have one AgentHTTPClient test working

e08c214

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

datadog: a few more AgentHTTPClient tests

1dee2ff

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

datadog: AgentHTTPClient unit tests

4cd97c9

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

datadog: consolidate unit test boilerplate

d600c11

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

datadog: more unit tests

52cdeee

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

Merge branch 'main' into david.goffredo/dd-trace-cpp-7/9-agent_http_c…

fd1620b

…lient

datadog: fix and comment and remove dead code

07d7d50

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

datadog: address review comments for AgentHTTPClient

9d8c148

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

datadog: more docs for EventScheduler::schedule_recurring_event

e3c8625

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

Merge branch 'main' into david.goffredo/dd-trace-cpp-6/9-event_scheduler

a709c34

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

datadog: set level in logger and sink in unit test

d6f92d8

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

datadog: add test to cover EventScheduler::config_json()

93e4020

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

dgoffredo added 4 commits March 14, 2023 18:25

datadog: breathing test for Tracer

3201d81

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

datadog: test "no-op mode" in Tracer

3ceb54d

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

datadog: more Tracer tests, update dd-trace-cpp

d91cd16

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

Merge branch 'main' into david.goffredo/dd-trace-cpp-8/9-tracer

23374bc

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

dgoffredo requested a review from mattklein123 as a code owner March 17, 2023 18:47

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

repokitteh-read-only Bot assigned mattklein123 Mar 17, 2023

dgoffredo mentioned this pull request Mar 17, 2023

Datadog tracing extension: remove OpenTracing #23958

Closed

mattklein123 assigned jmarantz and unassigned mattklein123 Mar 20, 2023

dgoffredo mentioned this pull request Mar 22, 2023

datadog: Config (final) portion of new tracing library #26284

Merged

jmarantz reviewed Mar 25, 2023

View reviewed changes

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

address some review comments

a7d5506

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

repokitteh-read-only Bot removed the waiting label Mar 27, 2023

jmarantz approved these changes Mar 30, 2023

View reviewed changes

jmarantz merged commit 37b4dff into envoyproxy:main Mar 30, 2023

Conversation

dgoffredo commented Mar 17, 2023

Uh oh!

repokitteh-read-only Bot commented Mar 17, 2023

Uh oh!

jmarantz commented Mar 23, 2023

Uh oh!

dgoffredo commented Mar 24, 2023

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!

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 commented Mar 27, 2023

Uh oh!

cgilmour commented Mar 30, 2023

Uh oh!

dgoffredo commented Mar 30, 2023

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Mar 30, 2023

Uh oh!

repokitteh-read-only Bot commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants