Implement telemetry API v2 #62

cgilmour · 2023-10-12T19:51:11Z

This implements an internal API that measures activity within the tracer and reports it to the Datadog Agent.
Users are able to disable this via the environment variable DD_INSTRUMENTATION_TELEMETRY_ENABLED or the report_telemetry config option in TracerConfig.

Additional metrics may be implemented in the future.

- mention new files in bazel build - don't store FinalizedTracerConfig - consistent spacing_style forMemberFunctions - make config_json() available to send_app_started() - fix unrelated pet peeve in use of log_startup - remove dev noise, which fixed all but one of the broken unit tests

…ETRY_ENABLED

…e of value()

…imeID

dgoffredo

Excellent!

Here's my first pass through the changes. See my comments inline. Overall:

good documentation
design and style are consistent with the rest of the library
test coverage is almost perfect on a line-by-line basis. You missed a spot.

I'll do another pass where I look at the unit tests in more detail, and the serialization code. And maybe poke around with an Agent proxy.

This is pretty much ready to merge, once we come to an agreement on ideas raised in the comments.

src/datadog/datadog_agent.cpp

src/datadog/metrics.h

src/datadog/tracer_telemetry.h

src/datadog/tracer_telemetry.cpp

test/mocks/http_clients.h

test/test_datadog_agent.cpp

…ry is making Curl requests.

pr-commenter · 2023-10-12T22:00:28Z

Benchmarks

Benchmark execution time: 2023-10-17 00:19:33

Comparing candidate commit c57a039 in PR branch cgilmour/telemetry-api with baseline commit 45c3c05 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

…lemetry-api

cgilmour · 2023-10-13T00:44:33Z

I've made the requested changes / fixes, and also merged in #61 because it was flaking for this PR as well.
Have another look when you have a chance.

dgoffredo

Looks good to me.

Thinking a bit more about the (remote?) possibility that this code gets deployed to a system with a Datadog Agent that's old enough not to have telemetry, do consider silencing or perhaps one-timing the error in telemetry_on_response_ when the response status is 404 ("error: Datadog Agent does not look like it supports telemetry", then subsequent silence), or something like that. Also reasonable would be to leave it as is.

Please see my comments about the type signatures of Metric's constructor parameters.

Aside from those ideas, ship it.

src/datadog/datadog_agent.cpp

dgoffredo · 2023-10-13T21:03:53Z

src/datadog/metrics.cpp

-Metric::Metric(const std::string name, std::string type,
-               const std::vector<std::string> tags, bool common)
+Metric::Metric(std::string name, std::string type,
+               std::vector<std::string> tags, bool common)
    : name_(name), type_(type), tags_(tags), common_(common) {}


Since we're accepting these parameters by value, might as well std::move them into these initializers.

Alternatively, use const T& instead.

Alternatively, use const vector& and StringView.

Alternatively, use std::move(vector) and StringView.

It's true that this code runs on tracer startup only, so it doesn't matter. But, style.

You might also just leave it like this. Less ink.

Now that I'm looking at the individual commits again, I wonder if the original const that you had on these parameters was a typo where you were missing the &. Anyway, figure out what you want the signatures and initializers to look like, based on how the constructor is called.

src/datadog/datadog_agent.cpp

dmehala · 2023-10-16T08:31:18Z

src/datadog/datadog_agent.cpp

      http_client_(config.http_client),
      event_scheduler_(config.event_scheduler),
      cancel_scheduled_flush_(event_scheduler_->schedule_recurring_event(
          config.flush_interval, [this]() { flush(); })),
      flush_interval_(config.flush_interval) {
  assert(logger_);
+  assert(tracer_telemetry_);
+  if (tracer_telemetry_->enabled()) {
+    // Only schedule this if telemetry is enabled.


Details but the line above is self explanatory.

dmehala · 2023-10-16T08:45:12Z

src/datadog/metrics.h

+  std::string name();
+  std::string type();
+  std::vector<std::string> tags();
+  bool common();
+  uint64_t value();


I will probably be controversial here but we could get rid of those accessors and put those members public 👀

value() hides the atomicity of the underlying member, for better or for worse.

My vote is to keep the trivial methods, but I thought the same as you when I initially read it.

No strong opinion about this, but I like the consistency and simplicity.
Have a go with the suggestion and see if you like it at the end.

src/datadog/runtime_id.h

src/datadog/tracer_telemetry.cpp

dmehala · 2023-10-16T11:42:32Z

src/datadog/tracer_telemetry.cpp

+    if (!points.empty()) {
+      auto type = metric.type();
+      if (type == "count") {
+        metrics.emplace_back(nlohmann::json::object({
+            {"metric", metric.name()},
+            {"tags", metric.tags()},
+            {"type", metric.type()},
+            {"points", points},
+            {"common", metric.common()},
+        }));
+      } else if (type == "gauge") {
+        // gauge metrics have a interval
+        metrics.emplace_back(nlohmann::json::object({
+            {"metric", metric.name()},
+            {"tags", metric.tags()},
+            {"type", metric.type()},
+            {"interval", 10},
+            {"points", points},
+            {"common", metric.common()},
+        }));
+      }


That could be done while capturing metrics so we avoid iterating twice every 60s and it will make heartbeat_and_telemetry easier to read and maintain.

It could, though I do like the simplicity of capture_metrics right now.
In the future if rate metrics, or distributions are added, it'll probably need refactoring.

I think we don't even use gauge currently. I assume Caleb left it in anticipating it would be used soon. Technically dead code, though, what do you think?

dmehala

LGTM

cgilmour and others added 27 commits October 3, 2023 03:48

WIP. Working API, limited metrics, missing tags for metrics

6618c75

Redesign to support tagged metrics

890490d

Add tags to metrics payloads. Ignore unused counters.

f41f4d4

Remove comforting developer noise

b91869f

Additions for building via bazel

0d4e82f

Adding comments

94bfc20

Store value of hostname in member field

9e432cb

Refactor runtime_id into span defaults, available to tracer telemetry

0005e65

Change count metrics to report absolute instead of cumulative value

e54bd30

Allow telemetry to be enabled/disabled using DD_INSTRUMENTATION_TELEM…

19e3e43

…ETRY_ENABLED

Add debug option, for developer convenience

5603789

Call capture_and_reset_value() instead of having reset behavior insid…

1fd88bd

…e of value()

Omit zero values, and sending empty generate-metrics payloads

8cf6d49

Comments

70ad77c

Add app-closing event

32b7885

Temporary fix for datadog agent tests

16d532b

Unit tests

9811f8c

CI reported this

dc94da4

Additional comments

1603c0e

Additional comments

09fc24d

Refactor repeated telemetry body construction

28cc724

Refactor http client callbacks

31d88a3

separate runtime_id from class SpanDefaults, and introduce class Runt…

879cfe3

…imeID

doc 'till you drop

26a5eac

don't use the real clock in SpanSampler's limiter test

03a9921

Disable telemetry for curl test that's intended for single requests

04e1a17

cgilmour requested a review from dgoffredo October 12, 2023 19:51

dgoffredo reviewed Oct 12, 2023

View reviewed changes

Add mutex to MockLogger, seems to be prevent SIGSEGV now that telemet…

688906d

…ry is making Curl requests.

cgilmour added 8 commits October 12, 2023 22:43

Bump coverage percentage

1ad8f70

Only pass this when calling methods

c839b3a

Log errors with a prefix

95c5d0b

Unconstification

5ec1769

Reordering things

a92cdf3

std::time_t

d9c2288

Change datadog_agent test

6e9e28d

Merge branch 'david.goffredo/fix-flaky-limiter-test' into cgilmour/te…

feee3fe

…lemetry-api

Only send interval for gauge metrics.

bdb9d9f

dgoffredo approved these changes Oct 13, 2023

View reviewed changes

dmehala reviewed Oct 16, 2023

View reviewed changes

src/datadog/datadog_agent.cpp Show resolved Hide resolved

dmehala reviewed Oct 16, 2023

View reviewed changes

src/datadog/runtime_id.h Show resolved Hide resolved

dmehala reviewed Oct 16, 2023

View reviewed changes

src/datadog/tracer_telemetry.cpp Show resolved Hide resolved

dmehala reviewed Oct 16, 2023

View reviewed changes

cgilmour added 3 commits October 16, 2023 23:46

Fix logged error message

28dd1c4

Fix the app-closing metrics payload

eb04fe8

Formatting.

c57a039

dmehala approved these changes Oct 17, 2023

View reviewed changes

cgilmour merged commit fd23d07 into main Oct 17, 2023

cgilmour deleted the cgilmour/telemetry-api branch October 17, 2023 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement telemetry API v2 #62

Implement telemetry API v2 #62

cgilmour commented Oct 12, 2023

dgoffredo left a comment

pr-commenter bot commented Oct 12, 2023 •

edited

Loading

cgilmour commented Oct 13, 2023

dgoffredo left a comment

dgoffredo Oct 13, 2023

dgoffredo Oct 13, 2023

dgoffredo Oct 13, 2023

dmehala Oct 16, 2023

dmehala Oct 16, 2023 •

edited

Loading

dgoffredo Oct 16, 2023

cgilmour Oct 16, 2023

dmehala Oct 16, 2023 •

edited

Loading

cgilmour Oct 17, 2023

dgoffredo Oct 17, 2023

dmehala left a comment

Implement telemetry API v2 #62

Implement telemetry API v2 #62

Conversation

cgilmour commented Oct 12, 2023

dgoffredo left a comment

Choose a reason for hiding this comment

pr-commenter bot commented Oct 12, 2023 • edited Loading

Benchmarks

cgilmour commented Oct 13, 2023

dgoffredo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmehala Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmehala Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmehala left a comment

Choose a reason for hiding this comment

pr-commenter bot commented Oct 12, 2023 •

edited

Loading

dmehala Oct 16, 2023 •

edited

Loading

dmehala Oct 16, 2023 •

edited

Loading