Skip to content

Commit c52dcf1

Browse files
authored
Merge pull request #3507 from DataDog/glopes/fix-duration_ext
appsec: fix duration_ext metric
2 parents e323e76 + d10e448 commit c52dcf1

File tree

8 files changed

+181
-31
lines changed

8 files changed

+181
-31
lines changed

appsec/src/extension/ddappsec.c

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "network.h"
3636
#include "php_compat.h"
3737
#include "php_objects.h"
38+
#include "rasp.h"
3839
#include "request_abort.h"
3940
#include "request_lifecycle.h"
4041
#include "tags.h"
@@ -219,6 +220,7 @@ static PHP_MINIT_FUNCTION(ddappsec)
219220
dd_user_tracking_startup();
220221
dd_request_abort_startup();
221222
dd_tags_startup();
223+
dd_rasp_startup();
222224
dd_ip_extraction_startup();
223225
dd_entity_body_startup();
224226
dd_backtrace_startup();
@@ -237,6 +239,7 @@ static PHP_MSHUTDOWN_FUNCTION(ddappsec)
237239
// no other thread is running now. reset config to global config only.
238240
runtime_config_first_init = false;
239241

242+
dd_rasp_shutdown();
240243
dd_tags_shutdown();
241244
dd_request_abort_shutdown();
242245
dd_user_tracking_shutdown();
@@ -486,9 +489,7 @@ static PHP_FUNCTION(datadog_appsec_testing_request_exec)
486489
static PHP_FUNCTION(datadog_appsec_push_addresses)
487490
{
488491
struct timespec start;
489-
struct timespec end;
490492
clock_gettime(CLOCK_MONOTONIC_RAW, &start);
491-
long elapsed = 0;
492493
UNUSED(return_value);
493494
if (!DDAPPSEC_G(active)) {
494495
mlog(dd_log_debug, "Trying to access to push_addresses "
@@ -522,16 +523,15 @@ static PHP_FUNCTION(datadog_appsec_push_addresses)
522523
dd_result res = dd_request_exec(conn, addresses, rasp_rule, &block_params);
523524

524525
if (rasp_rule && ZSTR_LEN(rasp_rule) > 0) {
526+
struct timespec end;
525527
clock_gettime(CLOCK_MONOTONIC_RAW, &end);
526-
elapsed =
527-
((int64_t)end.tv_sec - (int64_t)start.tv_sec) *
528+
double elapsed_us =
529+
((double)(end.tv_sec - start.tv_sec) *
530+
// NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
531+
(int64_t)1000000 +
528532
// NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
529-
(int64_t)1000000000 +
530-
((int64_t)end.tv_nsec - (int64_t)start.tv_nsec);
531-
zend_object *span = dd_trace_get_active_root_span();
532-
if (span) {
533-
dd_tags_add_rasp_duration_ext(span, elapsed);
534-
}
533+
(double)(end.tv_nsec - start.tv_nsec) / 1000.0);
534+
dd_rasp_account_duration_us(elapsed_us);
535535
}
536536

537537
dd_req_lifecycle_abort(REQUEST_STAGE_MID_REQUEST, res, &block_params);

appsec/src/extension/rasp.c

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
2+
#include "rasp.h"
3+
4+
#include "ddappsec.h"
5+
#include "ddtrace.h"
6+
#include "logging.h"
7+
#include "php_compat.h"
8+
#include "php_helpers.h"
9+
#include "request_lifecycle.h"
10+
#include "string_helpers.h"
11+
12+
#define DD_EVENTS_RASP_DURATION_EXT "_dd.appsec.rasp.duration_ext"
13+
14+
static zend_string *_dd_rasp_duration_ext;
15+
static THREAD_LOCAL_ON_ZTS double _duration_ext_us;
16+
17+
static void _add_rasp_duration_ext_to_metrics(
18+
zend_object *nonnull span, double duration);
19+
20+
void dd_rasp_startup(void)
21+
{
22+
_dd_rasp_duration_ext =
23+
zend_string_init_interned(LSTRARG(DD_EVENTS_RASP_DURATION_EXT), 1);
24+
}
25+
void dd_rasp_shutdown(void)
26+
{
27+
zend_string_release(_dd_rasp_duration_ext);
28+
_dd_rasp_duration_ext = NULL;
29+
}
30+
void dd_rasp_reset_globals(void) { _duration_ext_us = 0.0; }
31+
void dd_rasp_req_finish(void)
32+
{
33+
if (_duration_ext_us <= 0.0) {
34+
return;
35+
}
36+
zend_object *span = dd_req_lifecycle_get_cur_span();
37+
if (!span) {
38+
return;
39+
}
40+
_add_rasp_duration_ext_to_metrics(span, _duration_ext_us);
41+
_duration_ext_us = 0.0;
42+
}
43+
44+
void dd_rasp_account_duration_us(double duration_us)
45+
{
46+
_duration_ext_us += duration_us;
47+
}
48+
49+
static void _add_rasp_duration_ext_to_metrics(
50+
zend_object *nonnull span, double duration)
51+
{
52+
zval *metrics_zv = dd_trace_span_get_metrics(span);
53+
if (!metrics_zv) {
54+
return;
55+
}
56+
57+
zval zv;
58+
ZVAL_DOUBLE(&zv, duration);
59+
zval *nullable res =
60+
zend_hash_add(Z_ARRVAL_P(metrics_zv), _dd_rasp_duration_ext, &zv);
61+
if (res == NULL) {
62+
mlog(dd_log_warning, "Failed to add metric %.*s",
63+
(int)ZSTR_LEN(_dd_rasp_duration_ext),
64+
ZSTR_VAL(_dd_rasp_duration_ext));
65+
}
66+
}

appsec/src/extension/rasp.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
#pragma once
2+
3+
void dd_rasp_startup(void);
4+
void dd_rasp_shutdown(void);
5+
void dd_rasp_account_duration_us(double duration_us);
6+
void dd_rasp_reset_globals(void); // call on rinit/user req begin
7+
void dd_rasp_req_finish(void); // call on rshutdown/user req shutdown

appsec/src/extension/request_lifecycle.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "php_compat.h"
1515
#include "php_helpers.h"
1616
#include "php_objects.h"
17+
#include "rasp.h"
1718
#include "request_abort.h"
1819
#include "string_helpers.h"
1920
#include "tags.h"
@@ -345,6 +346,7 @@ static void _do_request_finish_php(bool ignore_verdict)
345346
dd_tags_add_tags(_cur_req_span, NULL);
346347
}
347348
dd_tags_rshutdown();
349+
dd_rasp_req_finish();
348350

349351
_reset_globals();
350352

@@ -391,6 +393,7 @@ static zend_array *_do_request_finish_user_req(bool ignore_verdict,
391393
if (DDAPPSEC_G(active) && _cur_req_span) {
392394
dd_tags_add_tags(_cur_req_span, superglob_equiv);
393395
}
396+
dd_rasp_req_finish();
394397

395398
zend_array *spec = dd_req_lifecycle_abort(
396399
REQUEST_STAGE_REQUEST_END, verdict, &ctx.req_info.block_params);
@@ -425,6 +428,7 @@ static void _reset_globals(void)
425428

426429
_shutdown_done_on_commit = false;
427430
dd_tags_rshutdown();
431+
dd_rasp_reset_globals();
428432
}
429433

430434
static zend_string *nullable _extract_ip_from_autoglobal(void)

appsec/src/extension/tags.c

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@
7676
"_dd.appsec.events.users.login.success.sdk"
7777
#define DD_EVENTS_USER_LOGIN_FAILURE_SDK \
7878
"_dd.appsec.events.users.login.failure.sdk"
79-
#define DD_EVENTS_RASP_DURATION_EXT "_dd.appsec.rasp.duration_ext"
8079
#define DD_SERVER_BUSINESS_LOGIC_USERS_SIGNUP \
8180
"server.business_logic.users.signup"
8281
#define DD_SERVER_BUSINESS_LOGIC_USERS_LOGIN_SUCCESS \
@@ -104,7 +103,6 @@ static zend_string *_dd_tag_rh_content_language; // response
104103
static zend_string *_dd_tag_user;
105104
static zend_string *_dd_tag_user_id;
106105
static zend_string *_dd_metric_enabled;
107-
static zend_string *_dd_rasp_duration_ext;
108106
static zend_string *_dd_business_logic_users_signup;
109107
static zend_string *_dd_business_logic_users_login_success;
110108
static zend_string *_dd_business_logic_users_login_failure;
@@ -217,9 +215,6 @@ void dd_tags_startup(void)
217215
_key_remote_addr_zstr =
218216
zend_string_init_interned(LSTRARG("REMOTE_ADDR"), 1);
219217

220-
_dd_rasp_duration_ext =
221-
zend_string_init_interned(LSTRARG(DD_EVENTS_RASP_DURATION_EXT), 1);
222-
223218
// Event related strings
224219
_track_zstr =
225220
zend_string_init_interned(LSTRARG("track"), 1 /* permanent */);
@@ -370,18 +365,6 @@ void dd_tags_set_event_user_id(zend_string *nonnull zstr)
370365
_event_user_id = zend_string_copy(zstr);
371366
}
372367

373-
void dd_tags_add_rasp_duration_ext(
374-
zend_object *nonnull span, zend_long duration)
375-
{
376-
zval *metrics_zv = dd_trace_span_get_metrics(span);
377-
if (!metrics_zv) {
378-
return;
379-
}
380-
zval zv;
381-
ZVAL_LONG(&zv, duration);
382-
zend_hash_add(Z_ARRVAL_P(metrics_zv), _dd_rasp_duration_ext, &zv);
383-
}
384-
385368
void dd_tags_rshutdown(void)
386369
{
387370
zend_llist_clean(&_appsec_json_frags);

appsec/src/extension/tags.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,3 @@ void dd_tags_set_event_user_id(zend_string *nonnull zstr);
2626

2727
// does not increase the refcount on zstr
2828
void dd_tags_add_appsec_json_frag(zend_string *nonnull zstr);
29-
30-
void dd_tags_add_rasp_duration_ext(
31-
zend_object *nonnull span, zend_long duration);

appsec/tests/extension/push_params_ok_04.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ bool(true)
3030
Array
3131
(
3232
[process_id] => %d
33-
[_dd.appsec.rasp.duration_ext] => %d
3433
[_dd.appsec.enabled] => %d
34+
[_dd.appsec.rasp.duration_ext] => %f
3535
)
3636
array(2) {
3737
[0]=>
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
--TEST--
2+
RASP duration_ext metric accumulates across multiple calls in user requests
3+
--INI--
4+
extension=ddtrace.so
5+
datadog.appsec.enabled=true
6+
datadog.appsec.cli_start_on_rinit=false
7+
--ENV--
8+
DD_TRACE_GENERATE_ROOT_SPAN=0
9+
--FILE--
10+
<?php
11+
12+
use function DDTrace\UserRequest\{notify_start,notify_commit};
13+
use function DDTrace\{start_span,close_span,active_span};
14+
use function datadog\appsec\push_addresses;
15+
16+
include __DIR__ . '/inc/mock_helper.php';
17+
18+
define('NUM_CALLS', 20);
19+
20+
$resps = array_merge(
21+
array(response_list(response_request_init([[['ok', []]], [], []]))),
22+
array_fill(0, NUM_CALLS,
23+
response_list(response_request_exec([[['ok', []]], [], [], [], false]))),
24+
array(
25+
response_list(response_request_shutdown([[['ok', []]], [], []])),
26+
27+
// Second user request
28+
response_list(response_request_init([[['ok', []]], [], []])),
29+
response_list(response_request_exec([[['ok', []]], [], [], [], false])),
30+
response_list(response_request_shutdown([[['ok', []]], [], []])),
31+
)
32+
);
33+
34+
$helper = Helper::createinitedRun($resps);
35+
36+
echo "=== First user request ===\n";
37+
$span1 = start_span();
38+
notify_start($span1, array());
39+
40+
for ($i = 0; $i < NUM_CALLS; $i++) {
41+
push_addresses(["server.request.path_params" => "test1"], "lfi");
42+
}
43+
44+
notify_commit($span1, 200, array());
45+
46+
// Get metrics for first request
47+
$metrics1 = $span1->metrics ?? [];
48+
echo "First request has duration_ext: " . (isset($metrics1['_dd.appsec.rasp.duration_ext']) ? "yes" : "no") . "\n";
49+
if (isset($metrics1['_dd.appsec.rasp.duration_ext'])) {
50+
$duration1 = $metrics1['_dd.appsec.rasp.duration_ext'];
51+
echo "First request duration_ext is positive: " . ($duration1 > 0 ? "yes" : "no") . "\n";
52+
}
53+
54+
close_span(100.0);
55+
56+
echo "\n=== Second user request ===\n";
57+
$span2 = start_span();
58+
notify_start($span2, array());
59+
60+
push_addresses(["server.request.body" => "test3"], "lfi");
61+
62+
notify_commit($span2, 200, array());
63+
64+
// Get metrics for second request
65+
$metrics2 = $span2->metrics ?? [];
66+
echo "Second request has duration_ext: " . (isset($metrics2['_dd.appsec.rasp.duration_ext']) ? "yes" : "no") . "\n";
67+
if (isset($metrics2['_dd.appsec.rasp.duration_ext'])) {
68+
$duration2 = $metrics2['_dd.appsec.rasp.duration_ext'];
69+
echo "Second request duration_ext is positive: " . ($duration2 > 0 ? "yes" : "no") . "\n";
70+
}
71+
72+
close_span(100.0);
73+
74+
if (isset($duration1) && isset($duration2)) {
75+
echo "\nBoth requests have duration_ext metrics: yes\n";
76+
if ($duration1 > $duration2) {
77+
echo "First request has a larger duration_ext: yes\n";
78+
}
79+
}
80+
81+
82+
?>
83+
--EXPECTF--
84+
=== First user request ===
85+
First request has duration_ext: yes
86+
First request duration_ext is positive: yes
87+
88+
=== Second user request ===
89+
Second request has duration_ext: yes
90+
Second request duration_ext is positive: yes
91+
92+
Both requests have duration_ext metrics: yes
93+
First request has a larger duration_ext: yes

0 commit comments

Comments
 (0)