Skip to content

Conversation

@dmehala
Copy link
Collaborator

@dmehala dmehala commented Mar 31, 2025

Description

This is Part 3 of the telemetry module refactoring. This PR introduces a singleton for the telemetry module, allowing external usage outside of the tracer API while ensuring all components access the same instance. Additionally, this change simplifies internal telemetry usage.

For reference:

@dmehala dmehala requested a review from a team as a code owner March 31, 2025 09:31
@dmehala dmehala requested review from dubloom, pablomartinezbernardo and zacharycmontoya and removed request for a team March 31, 2025 09:31
@pr-commenter
Copy link

pr-commenter bot commented Apr 2, 2025

Benchmarks

Benchmark execution time: 2025-04-03 10:07:39

Comparing candidate commit 6b62265 in PR branch dmehala/telemetry/part-3-introduce-singleton with baseline commit 5013321 in branch main.

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

Copy link
Contributor

@dubloom dubloom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just some small comments

std::visit(
details::Overload{
[&](Telemetry& telemetry) { telemetry.send_app_started(conf); },
[](NoopTelemetry) {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[](NoopTelemetry) {},
[](auto&&) {},

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's on purpose. While auto&& acts as a sink, I prefer the explicitness of the implementation. It reduces the risk of errors if someone, for any reason, decides to add a new value to the variant.

Copy link
Contributor

@dubloom dubloom Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to do the same in the below implementations ? Or the goal is to show the two versions ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that's a mistake on my end

Base automatically changed from dmehala/telemetry/part-2-move-telemetry-exporter to main April 3, 2025 09:56
dmehala and others added 2 commits April 3, 2025 11:58
The introduction of a singleton for the telemetry modules allow external usage
outside of the tracer API to access the same instance. This change also simplified
internal telemetry usage.
Co-authored-by: pablomartinezbernardo <134320516+pablomartinezbernardo@users.noreply.github.com>
@dmehala dmehala force-pushed the dmehala/telemetry/part-3-introduce-singleton branch from b37bfdc to 36a0cd6 Compare April 3, 2025 09:59
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 55.27273% with 123 lines in your changes missing coverage. Please review.

Project coverage is 90.30%. Comparing base (5013321) to head (6b62265).

Files with missing lines Patch % Lines
src/datadog/telemetry/telemetry_impl.cpp 56.49% 77 Missing ⚠️
src/datadog/telemetry/telemetry.cpp 40.00% 27 Missing ⚠️
include/datadog/telemetry/metrics.h 0.00% 15 Missing ⚠️
src/datadog/datadog_agent.cpp 81.81% 2 Missing ⚠️
src/datadog/tracer.cpp 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
- Coverage   93.38%   90.30%   -3.08%     
==========================================
  Files          78       80       +2     
  Lines        4487     4621     +134     
==========================================
- Hits         4190     4173      -17     
- Misses        297      448     +151     

☔ 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.

@dmehala dmehala merged commit c6ca2f0 into main Apr 3, 2025
22 checks passed
@dmehala dmehala deleted the dmehala/telemetry/part-3-introduce-singleton branch April 3, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants