Enable sidecar by default on PHP 8.3 #2680

pierotibou · 2024-05-29T21:51:15Z

Description

This PR enables sending traces on Unix through the sidecar by default in PHP 8.3.
This will thus impact 5% of our customers.

If the customer has explicitly disabled telemetry (thus the sidecar), we need to fallback to the background zender.

This is tested through unit tests on config logging.

APMSP-1154

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-05-29T21:57:22Z

Codecov Report

Attention: Patch coverage is 95.31250% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (c795fcd) to head (1085733).

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2680      +/-   ##
============================================
- Coverage     79.40%   77.96%   -1.44%     
  Complexity     2212     2212              
============================================
  Files           201      227      +26     
  Lines         22488    26551    +4063     
  Branches          0      988     +988     
============================================
+ Hits          17857    20701    +2844     
- Misses         4631     5324     +693     
- Partials          0      526     +526

Flag	Coverage Δ
appsec-extension	`69.13% <ø> (?)`
tracer-extension	`78.86% <95.31%> (+0.23%)`	⬆️
tracer-php	`80.52% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
ext/coms.c	`79.64% <100.00%> (+0.70%)`	⬆️
ext/coms.h	`100.00% <ø> (ø)`
ext/configuration.h	`100.00% <ø> (ø)`
ext/startup_logging.c	`86.06% <100.00%> (+0.14%)`	⬆️
ext/auto_flush.c	`71.91% <0.00%> (+17.97%)`	⬆️
ext/ddtrace.c	`74.36% <94.11%> (+0.22%)`	⬆️
ext/serializer.c	`82.18% <87.50%> (+0.54%)`	⬆️

... and 26 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c795fcd...1085733. Read the comment docs.

pr-commenter · 2024-05-30T12:57:13Z

Benchmarks

Benchmark execution time: 2024-06-07 21:43:42

Comparing candidate commit fa07162 in PR branch pierre/enable-sidecar-by-default with baseline commit c795fcd in branch master.

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

scenario:ComposerTelemetryBench/benchTelemetryParsing

🟩 execution_time [-3.072µs; -1.328µs] or [-5.197%; -2.248%]

scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache

🟩 execution_time [-4.586µs; -1.814µs] or [-7.291%; -2.884%]

scenario:ContextPropagationBench/benchInject64Bit

🟩 execution_time [-465.393ns; -190.607ns] or [-5.970%; -2.445%]

scenario:ContextPropagationBench/benchInject64Bit-opcache

🟩 execution_time [-515.418ns; -206.582ns] or [-6.602%; -2.646%]

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

🟩 execution_time [-192.105µs; -57.535µs] or [-7.485%; -2.242%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization

🟩 execution_time [-5.762µs; -4.478µs] or [-3.793%; -2.948%]

bantonsson · 2024-06-05T07:34:01Z

ext/ddtrace.c

-                           get_global_DD_TRACE_AGENT_STACK_BACKLOG());
-    }
-#endif
-


The code paths are just to complicated for my brain, but why is it safe to move this code to dd_activate_once?

Background sender thread is only actually needed once spans may actually be flushed. And that never happens before dd_activate_once.

iamluc · 2024-06-05T07:46:12Z

ext/ddtrace.h

@@ -64,6 +64,10 @@ void dd_internal_handle_fork(void);
 void dd_run_rust_thread_destructors(void *unused);
 #endif

+#ifndef _WIN32
+void disable_sidecar_sending(void);


nitpick: this function does not seem to be used elsewhere but in ddtrace.c. So you could declare it before its usage (+ make it static) and remove the declaration from here

iamluc · 2024-06-05T07:52:43Z

tests/ext/sidecar_enabled.phpt

+--TEST--
+Sidecar should be enabled by default on PHP 8.3
+--SKIPIF--
+<?php include 'startup_logging_skipif_unix_83.inc'; ?>


The name is a bit confusing. What about startup_logging_skip_unless_unix_83?

Also, what about moving the files to the includes directory

pierotibou

LGTM functionnaly speaking.
Thanks a lot for going through with finishing implementation and adding telemetry

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Manually set sidecar sender config. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

pierotibou

Cannot approve my own PR (even though, it's clearly not my PR anymore :p)

Overall LGTM, I just have a few questions. Also realizing that the sidecar sets _top_level I would have rather not if not agentless.

pierotibou · 2024-06-07T20:23:40Z

tests/Integration/LongRunning/LongRunningScriptTest.php

@@ -42,19 +47,19 @@ public function testTracesFromLongRunningFunctionWithMixedTracing()
        $rootTraceAssertion = function ($i) {
            return SpanAssertion::exists("do_manual_instrumentation_within_root_trace_function", "run $i")
                ->withChildren([
-                    SpanAssertion::exists("first-sub-operation")->withChildren(


I don't get why those have changed

normalization changes. Do you want to revert it?

I'd rather not mess around unnecessarily by now.

tests/Integrations/PCNTL/PCNTLTest.php

pierotibou · 2024-06-07T20:30:57Z

tests/Common/SpanChecker.php

@@ -540,6 +540,10 @@ function ($key) use ($pattern) {
            if (isset($spanMetrics["process_id"])) {
                unset($spanMetrics['process_id']);
            }
+            if (isset($spanMetrics["_top_level"])) {
+                // Set by sidecar only
+                unset($spanMetrics['_top_level']);


The sidecar, if not agentless, shouldn't set, for the sake of consistency with the other tracers.

Uh, I thought _top_level is an attribute of partial flushing anyway - e.g. java is setting it: https://github.com/DataDog/dd-trace-java/blob/fff0006e3844272fc7bc711117b15dfd87a8feee/dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV0_5.java#L258

Java is setting _dd.top_level, this is one set only by the agent

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

bwoebi

As Pierre cannot self-approve :-)

This reverts commit c5f2dcb.

* Enable sidecar by default, only when telemetry is enabled * Manually set sidecar config to false Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * minor comments in the tests * add a test to make sure sidecar is enabled by default in 8.3 * Delay bgs initialization until sidecar sender status is finalized Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix crash Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Add telemetry for telemetry disabled Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix mem leak Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Hack to massage sidecar v0.7 into what is expected by tests Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix telemetry path Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * More test stability Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Fix header name * Update libdatadog * Test fixes Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * In the agent sampling test, telemetry conflicts with sidecar Manually set sidecar sender config. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Avoid parseMultipleRequestsFromDumpedData with sidecar * Skip valgrind test Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Turn sidecar sender off in valgrind tests because of false positives Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Björn Antonsson <bjorn.antonsson@datadoghq.com> Co-authored-by: Luc Vieillescazes <luc.vieillescazes@datadoghq.com>

bwoebi force-pushed the pierre/enable-sidecar-by-default branch from 17f3a13 to b5e4ec2 Compare May 30, 2024 09:59

pierotibou force-pushed the pierre/enable-sidecar-by-default branch from 4f72426 to bc1fa68 Compare May 30, 2024 12:28

bwoebi force-pushed the pierre/enable-sidecar-by-default branch from 96cde75 to 0344bb0 Compare June 3, 2024 12:40

pierotibou marked this pull request as ready for review June 4, 2024 18:11

pierotibou requested review from a team as code owners June 4, 2024 18:11

bantonsson reviewed Jun 5, 2024

View reviewed changes

iamluc reviewed Jun 5, 2024

View reviewed changes

bwoebi force-pushed the pierre/enable-sidecar-by-default branch from 0344bb0 to 46ca434 Compare June 6, 2024 00:51

pierotibou commented Jun 6, 2024

View reviewed changes

bwoebi mentioned this pull request Jun 6, 2024

Hack to massage sidecar v0.7 into what is expected by tests #2694

Closed

2 tasks

bwoebi force-pushed the pierre/enable-sidecar-by-default branch 7 times, most recently from 90f1fef to c4d648a Compare June 7, 2024 18:30

pierotibou and others added 9 commits June 7, 2024 21:47

Enable sidecar by default, only when telemetry is enabled

dcd29c1

Manually set sidecar config to false

4194c49

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

minor comments in the tests

f9dc45f

add a test to make sure sidecar is enabled by default in 8.3

b41062a

Delay bgs initialization until sidecar sender status is finalized

6a5ea16

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Fix crash

fa95c12

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Add telemetry for telemetry disabled

05307d2

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Fix mem leak

f8d1f1b

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Hack to massage sidecar v0.7 into what is expected by tests

34fe63f

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

bwoebi and others added 7 commits June 7, 2024 21:47

Fix telemetry path

63202de

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

More test stability

58845c9

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Fix header name

a55c362

Update libdatadog

d88c7fc

Test fixes

67135e5

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

In the agent sampling test, telemetry conflicts with sidecar

8079e32

Manually set sidecar sender config. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Avoid parseMultipleRequestsFromDumpedData with sidecar

017a3d2

bwoebi force-pushed the pierre/enable-sidecar-by-default branch from c4d648a to 446a704 Compare June 7, 2024 19:49

Skip valgrind test

a0edcc0

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

bwoebi force-pushed the pierre/enable-sidecar-by-default branch from 446a704 to a0edcc0 Compare June 7, 2024 20:11

pierotibou commented Jun 7, 2024

View reviewed changes

Turn sidecar sender off in valgrind tests because of false positives

1085733

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

bwoebi force-pushed the pierre/enable-sidecar-by-default branch from fa07162 to 1085733 Compare June 7, 2024 21:55

bwoebi approved these changes Jun 7, 2024

View reviewed changes

bwoebi merged commit c5f2dcb into master Jun 7, 2024
600 of 606 checks passed

bwoebi deleted the pierre/enable-sidecar-by-default branch June 7, 2024 23:02

github-actions bot added this to the 1.1.0 milestone Jun 7, 2024

pierotibou added a commit that referenced this pull request Jun 10, 2024

Revert "Enable sidecar by default on PHP 8.3 (#2680)"

c43f712

This reverts commit c5f2dcb.

pierotibou mentioned this pull request Jun 10, 2024

Revert "Enable sidecar by default on PHP 8.3 (#2680)" #2699

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable sidecar by default on PHP 8.3 #2680

Enable sidecar by default on PHP 8.3 #2680

pierotibou commented May 29, 2024 •

edited

Loading

codecov-commenter commented May 29, 2024 •

edited

Loading

pr-commenter bot commented May 30, 2024 •

edited

Loading

bantonsson Jun 5, 2024

bwoebi Jun 5, 2024

iamluc Jun 5, 2024

iamluc Jun 5, 2024

pierotibou left a comment •

edited

Loading

pierotibou left a comment

pierotibou Jun 7, 2024

bwoebi Jun 7, 2024

bwoebi Jun 7, 2024

pierotibou Jun 7, 2024

bwoebi Jun 7, 2024

pierotibou Jun 7, 2024

bwoebi left a comment

Enable sidecar by default on PHP 8.3 #2680

Enable sidecar by default on PHP 8.3 #2680

Conversation

pierotibou commented May 29, 2024 • edited Loading

Description

Reviewer checklist

codecov-commenter commented May 29, 2024 • edited Loading

Codecov Report

pr-commenter bot commented May 30, 2024 • edited Loading

Benchmarks

scenario:ComposerTelemetryBench/benchTelemetryParsing

scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache

scenario:ContextPropagationBench/benchInject64Bit

scenario:ContextPropagationBench/benchInject64Bit-opcache

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

scenario:MessagePackSerializationBench/benchMessagePackSerialization

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

pierotibou left a comment • edited Loading

Choose a reason for hiding this comment

pierotibou 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwoebi left a comment

Choose a reason for hiding this comment

pierotibou commented May 29, 2024 •

edited

Loading

codecov-commenter commented May 29, 2024 •

edited

Loading

pr-commenter bot commented May 30, 2024 •

edited

Loading

pierotibou left a comment •

edited

Loading