[part 6] refactor!(telemetry): remove dependency on `TracerTelemetry` by dmehala · Pull Request #204 · DataDog/dd-trace-cpp

dmehala · 2025-04-07T07:21:50Z

Description

Moves the TracerTelemetry logic directly into Telemetry, eliminating the need for a separate TracerTelemetry object.

Previously, telemetry metrics were collected every 10 seconds and heartbeats were sent every 60 seconds, matching the default configuration. However, these intervals can be customized via environment variables or code. This commit updates the telemetry logic to use the configured intervals—whether set through environment variables or directly in the code—ensuring that metrics , heartbeat and logs messages are sent accordingly.

Moves the TracerTelemetry logic directly into Telemetry, eliminating the need for a separate `TracerTelemetry` object.

pr-commenter · 2025-04-07T08:25:52Z

Benchmarks

Benchmark execution time: 2025-04-08 11:06:29

Comparing candidate commit baafa6e in PR branch dmehala/telemetry/part-6-nuke-tracer-telemetry with baseline commit e777070 in branch main.

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

codecov-commenter · 2025-04-07T08:29:01Z

Codecov Report

Attention: Patch coverage is 61.89189% with 141 lines in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (e777070) to head (baafa6e).

Files with missing lines	Patch %	Lines
src/datadog/telemetry/telemetry_impl.cpp	61.89%	141 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #204      +/-   ##
==========================================
- Coverage   90.81%   90.21%   -0.60%     
==========================================
  Files          81       79       -2     
  Lines        4649     4672      +23     
==========================================
- Hits         4222     4215       -7     
- Misses        427      457      +30

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pablomartinezbernardo · 2025-04-07T08:27:28Z

src/datadog/telemetry/telemetry_impl.cpp

+  // Register all the metrics that we're tracking by adding them to the
+  // metrics_snapshots_ container. This allows for simpler iteration logic
+  // when using the values in `generate-metrics` messages.
+  metrics_snapshots_.emplace_back(metrics_.tracer.spans_created,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.tracer.spans_finished,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.tracer.trace_segments_created_new,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(
+      metrics_.tracer.trace_segments_created_continued, MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.tracer.trace_segments_closed,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.requests,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.responses_1xx,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.responses_2xx,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.responses_3xx,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.responses_4xx,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.responses_5xx,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.errors_timeout,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.errors_network,
+                                  MetricSnapshot{});
+  metrics_snapshots_.emplace_back(metrics_.trace_api.errors_status_code,
+                                  MetricSnapshot{});
+
+  for (auto& m : user_metrics_) {
+    metrics_snapshots_.emplace_back(*m, MetricSnapshot{});
+  }


Can we create a helper method for this?

This is temporary and is expected to be removed in an upcoming PR.

pablomartinezbernardo · 2025-04-07T08:33:15Z

src/datadog/telemetry/telemetry_impl.cpp

+    for (const auto& log : logs_) {
+      auto encoded = encode_log(log);
+      encoded_logs.emplace_back(std::move(encoded));
+    }


Should logs_ be cleared after this?

Indeed it should. Thanks.

Same for metrics, no ?

No, that's correctly handled by metrics

pablomartinezbernardo · 2025-04-07T08:36:03Z

src/datadog/telemetry/telemetry_impl.cpp

                     std::shared_ptr<tracing::Logger> logger,
                     std::shared_ptr<tracing::HTTPClient> client,
-                     std::vector<std::shared_ptr<Metric>> metrics,
+                     std::vector<std::shared_ptr<Metric>> user_metrics_,


We should change to user_metrics and initialize the member variable

addressed in baafa6e

dubloom

LGTM except few comments

dubloom · 2025-04-08T11:36:53Z

src/datadog/telemetry/telemetry_impl.cpp

+  }
+}
+
+void Telemetry::log(std::string message, telemetry::LogLevel level,


Out of curiosity, why did you remove the inline ?

The main reason for this is that the definition is now in the .cpp file. If the question is why has it move in the .cpp, then, it's mainly to have all definitions in one place rather than scattered across the .h and .cpp. Do that answer your question?

Yep, thank you

dubloom · 2025-04-08T11:41:42Z

src/datadog/telemetry/telemetry_impl.cpp

+    for (const auto& log : logs_) {
+      auto encoded = encode_log(log);
+      encoded_logs.emplace_back(std::move(encoded));
+    }


Same for metrics, no ?

dubloom · 2025-04-08T11:42:41Z

src/datadog/telemetry/telemetry_impl.cpp

+    for (const auto& log : logs_) {
+      auto encoded = encode_log(log);
+      encoded_logs.emplace_back(std::move(encoded));
+    }


This function body looks very similar to the above function body. Would it be worth to refactor that ?

That's planned but not now.

dmehala added 2 commits April 5, 2025 13:21

[part 6] refactor!(telemetry): remove dependency on TracerTelemetry

fa58332

Moves the TracerTelemetry logic directly into Telemetry, eliminating the need for a separate `TracerTelemetry` object.

dmehala requested a review from a team as a code owner April 7, 2025 07:21

dmehala requested review from dubloom, pablomartinezbernardo and zacharycmontoya and removed request for a team April 7, 2025 07:21

init host info

c5290ff

pablomartinezbernardo reviewed Apr 7, 2025

View reviewed changes

Base automatically changed from dmehala/telemetry/part-5-heartbeat to main April 7, 2025 17:58

dmehala added 2 commits April 8, 2025 12:43

Merge branch 'main' into dmehala/telemetry/part-6-nuke-tracer-telemetry

c4ae040

code review

baafa6e

dmehala requested a review from pablomartinezbernardo April 8, 2025 11:04

dubloom approved these changes Apr 8, 2025

View reviewed changes

dmehala merged commit c85660b into main Apr 8, 2025
22 checks passed

dmehala deleted the dmehala/telemetry/part-6-nuke-tracer-telemetry branch April 8, 2025 12:44

dmehala mentioned this pull request Apr 8, 2025

[part 7] refactor!(telemetry): improve telemetry lifecycle #205

Merged

Conversation

dmehala commented Apr 7, 2025

Description

Uh oh!

pr-commenter bot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

codecov-commenter commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

dubloom 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pr-commenter bot commented Apr 7, 2025 •

edited

Loading

codecov-commenter commented Apr 7, 2025 •

edited

Loading