RUM telemetry #204

pablomartinezbernardo · 2025-04-25T08:03:52Z

Add RUM telemetry:

All already existing metrics
Including CSP
Application id and remote config tags

codecov-commenter · 2025-04-25T08:35:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.25%. Comparing base (a2e2d29) to head (e9bb286).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #204   +/-   ##
=======================================
  Coverage   71.25%   71.25%           
=======================================
  Files          49       49           
  Lines        6356     6356           
  Branches      903      903           
=======================================
  Hits         4529     4529           
  Misses       1378     1378           
  Partials      449      449

Files with missing lines	Coverage Δ
src/datadog_context.cpp	`61.79% <ø> (ø)`
src/ngx_http_datadog_module.cpp	`70.11% <ø> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dmehala · 2025-04-26T07:28:33Z

src/rum/telemetry.h

+extern datadog::telemetry::Counter injection_skipped;
+extern datadog::telemetry::Counter injection_succeed;
+extern datadog::telemetry::Counter injection_failed;
+extern datadog::telemetry::Counter content_security_policy;


Could you add const here to clarify that this isn't intended to be modified?

dmehala · 2025-04-26T07:29:05Z

src/rum/telemetry.h

+
+const std::vector<std::string>& get_common_tags();
+
+void increment_counter(const datadog::telemetry::Counter& counter,


To make it clearer for the reader, it would be better to avoid reusing the same telemetry function name. Instead, you could expose a function to build the telemetry tags, for example

template <typename... T> auto build_telemetry_tags(T&&... tags) { std::vector<std::string> tags{std::forward<T>(args)...}; tags.emplace_back("integration_name:nginx"); ... return tags; }

dmehala · 2025-04-26T07:36:24Z

src/rum/injection.h

@@ -32,6 +32,7 @@ class InjectionHandler final {

  // A flag indicating whether padding should be added to the HTML responses.
  bool output_padding_;
+  bool rum_first_csp_ = true;


Remove once getteing the csp header is done after process the request.

dmehala · 2025-04-26T07:39:59Z

src/rum/injection.cpp

+    telemetry::increment_counter(telemetry::injection_failed,
+                                 {"reason:missing_header_tag",
+                                  "application_id:" + cfg->rum_application_id,
+                                  "remote_config_used:" + cfg->remote_config});


Consider building the application_id and remote_config_used tags once, rather than recreating them each time.

dmehala · 2025-04-26T07:56:31Z

src/rum/injection.cpp

+  if (rum_first_csp_) {
+    if (auto csp =
+            search_header(r->headers_out.headers, "content-security-policy");
+        csp != nullptr) {
+      telemetry::increment_counter(telemetry::content_security_policy,
+                                   {"status:seen", "kind:header"});
+    }
+    rum_first_csp_ = false;
+  }


It would be better to move this to after the request is fully processed, inside on_log_request.

dmehala · 2025-04-26T07:59:30Z

src/datadog_conf.h

+  std::string rum_application_id;
+  std::string remote_config;


consider using these to build the tag, and renaming it to something more descriptive to make it clearer for the reader. also consider initializing these.

dmehala · 2025-04-26T08:00:13Z

src/rum/config.cpp

+  loc_conf->rum_application_id = "";
+  if (rum_config.count("applicationId") > 0 &&
+      !rum_config["applicationId"].empty()) {
+    loc_conf->rum_application_id = rum_config["applicationId"][0];
+  }
+
+  loc_conf->remote_config = (rum_config.count("remoteConfigurationId") > 0 &&
+                             !rum_config["remoteConfigurationId"].empty())
+                                ? "true"
+                                : "false";


It might be better to use find here instead. Since these are only used for telemetry tags, consider building the tags directly here or at each call to telemetry::counter::X>

dmehala

Add few comments that should be addressed. This PR has a dependency on dd-trace-cpp. Please note that it cannot be approved/merged until dd-trace-cpp is upgraded.

dmehala · 2025-04-28T12:16:41Z

src/rum/config.cpp

+  auto it_app_id = rum_config.find("applicationId");
+  loc_conf->rum_application_id_tag =
+      "application_id:" +
+      ((it_app_id != rum_config.end() && !it_app_id->second.empty())
+           ? it_app_id->second[0]
+           : std::string("N/A"));
+
+  auto it_remote_config = rum_config.find("remoteConfigurationId");
+  bool remote_config_present = (it_remote_config != rum_config.end() &&
+                                !it_remote_config->second.empty());
+  loc_conf->rum_remote_config_tag =
+      "remote_config_used:" +
+      std::string(remote_config_present ? "true" : "false");
+


Suggested change

auto it_app_id = rum_config.find("applicationId");

loc_conf->rum_application_id_tag =

"application_id:" +

((it_app_id != rum_config.end() && !it_app_id->second.empty())

? it_app_id->second[0]

: std::string("N/A"));

auto it_remote_config = rum_config.find("remoteConfigurationId");

bool remote_config_present = (it_remote_config != rum_config.end() &&

!it_remote_config->second.empty());

loc_conf->rum_remote_config_tag =

"remote_config_used:" +

std::string(remote_config_present ? "true" : "false");

loc_conf->rum_application_id_tag = "application_id:N/A";

loc_conf->rum_remote_config_tag = "remote_config_used:false";

if (auto it = rum_config.find("applicationId"); it != rum_config.cend() && !it->second.empty()) {

loc_conf->rum_application_id_tag = "application_id:" + it_app_id->second;

}

if (auto it = rum_config.find("remoteConfigurationId"); it != rum_config.cend() && !it->second.empty()) {

loc_conf->rum_remote_config_tag = true;

}

I would even argue to set the default rum_application_id_tag and rum_remote_config_tag during the loc_conf init.

dmehala · 2025-04-28T12:19:03Z

src/rum/injection.cpp

  }

  return output(r, output_chain, next_body_filter);
 }

+ngx_int_t InjectionHandler::on_log_request(ngx_http_request_t *r) {
+  if (rum_first_csp_) {


IIRC on_log_request is designed to be called only once per request. May I suggest to remove this check?

dmehala · 2025-04-28T12:19:46Z

src/rum/injection.h

+  // @param r - HTTP request being processed.
+  ngx_int_t on_log_request(ngx_http_request_t *r);


Suggested change

// @param r - HTTP request being processed.

ngx_int_t on_log_request(ngx_http_request_t *r);

// @param r - HTTP request being processed.

// @return ngx_int_t - Status code indicating success or failure.

ngx_int_t on_log_request(ngx_http_request_t *r);

dmehala · 2025-04-28T12:23:01Z

src/rum/telemetry.h

+  static const std::vector<std::string> common_tags = {
+      "integration_name:nginx",
+      std::format("integration_version:{}",
+                  std::string(datadog_semver_nginx_mod)),
+      "injector_version:0.1.0"};
+
+  std::vector<std::string> tags{std::forward<T>(specific_tags)...};
+
+  tags.reserve(tags.size() + common_tags.size());
+  tags.insert(tags.end(), common_tags.begin(), common_tags.end());


Suggested change

static const std::vector<std::string> common_tags = {

"integration_name:nginx",

std::format("integration_version:{}",

std::string(datadog_semver_nginx_mod)),

"injector_version:0.1.0"};

std::vector<std::string> tags{std::forward<T>(specific_tags)...};

tags.reserve(tags.size() + common_tags.size());

tags.insert(tags.end(), common_tags.begin(), common_tags.end());

std::vector<std::string> tags{std::forward<T>(specific_tags)...};

tags.emplace_back("integration_name:nginx");

tags.emplace_back("injector_version:0.1.0");

tags.emplace_back(std::format("integration_version:{}", datadog_semver_nginx_mod.c_str()));

dmehala · 2025-04-28T12:27:41Z

src/rum/telemetry.h

+namespace telemetry {
+
+template <typename... T>
+auto build_telemetry_tags(T&&... specific_tags) {


I noticed that the term 'telemetry' appears both in the name and the namespace, which might be redundant. Would you consider renaming it to build_tags to improve clarity?

pablomartinezbernardo · 2025-04-29T10:02:00Z

All comments addressed

Add few comments that should be addressed. This PR has a dependency on dd-trace-cpp. Please note that it cannot be approved/merged until dd-trace-cpp is upgraded.

Does this mean you are expecting dd-trace-cpp to be upgraded in this same PR? Or that given there's ongoing work in dd-trace-cpp then this has to wait?

Given RUM is not being built by default, I was leaving upgrading dd-trace-cpp up to you, and upgrading it only in the RUM branch testing it for RUM.

pablomartinezbernardo marked this pull request as ready for review April 25, 2025 12:52

pablomartinezbernardo requested a review from a team as a code owner April 25, 2025 12:52

pablomartinezbernardo requested review from dmehala and removed request for a team April 25, 2025 12:52

dmehala reviewed Apr 27, 2025

View reviewed changes

pablomartinezbernardo marked this pull request as draft April 28, 2025 08:40

pablomartinezbernardo force-pushed the pmartinez/rum-app-id-rc-tags branch from 517011d to 7b98a5d Compare April 28, 2025 09:50

pablomartinezbernardo marked this pull request as ready for review April 28, 2025 09:54

pablomartinezbernardo requested a review from dmehala April 28, 2025 09:54

dmehala reviewed Apr 28, 2025

View reviewed changes

amortemousque approved these changes Apr 30, 2025

View reviewed changes

RUM telemetry

e9bb286

pablomartinezbernardo force-pushed the pmartinez/rum-app-id-rc-tags branch from 1e3fee9 to e9bb286 Compare May 5, 2025 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM telemetry #204

RUM telemetry #204

pablomartinezbernardo commented Apr 25, 2025 •

edited

Loading

codecov-commenter commented Apr 25, 2025 •

edited

Loading

dmehala Apr 26, 2025 •

edited

Loading

dmehala Apr 26, 2025

dmehala Apr 26, 2025

dmehala Apr 26, 2025

dmehala Apr 26, 2025

dmehala Apr 26, 2025

dmehala Apr 26, 2025

dmehala left a comment •

edited

Loading

dmehala Apr 28, 2025

dmehala Apr 28, 2025

dmehala Apr 28, 2025

dmehala Apr 28, 2025

dmehala Apr 28, 2025

pablomartinezbernardo commented Apr 29, 2025


		const std::vector<std::string>& get_common_tags();

		void increment_counter(const datadog::telemetry::Counter& counter,

		// @param r - HTTP request being processed.
		ngx_int_t on_log_request(ngx_http_request_t *r);

RUM telemetry #204

Are you sure you want to change the base?

RUM telemetry #204

Conversation

pablomartinezbernardo commented Apr 25, 2025 • edited Loading

codecov-commenter commented Apr 25, 2025 • edited Loading

Codecov Report

dmehala Apr 26, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmehala left a comment • edited Loading

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

Choose a reason for hiding this comment

pablomartinezbernardo commented Apr 29, 2025

pablomartinezbernardo commented Apr 25, 2025 •

edited

Loading

codecov-commenter commented Apr 25, 2025 •

edited

Loading

dmehala Apr 26, 2025 •

edited

Loading

dmehala left a comment •

edited

Loading