Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement UST Consistency #2896

Merged
merged 12 commits into from
Nov 4, 2024

Conversation

khanayan123
Copy link
Contributor

@khanayan123 khanayan123 commented Oct 15, 2024

Description

Make changes such that iff a span has service name set by DD_SERVICE, it also gets the version specified in DD_VERSION. This is done to better align with the UST spec https://docs.datadoghq.com/getting_started/tagging/unified_service_tagging/?tab=kubernetes

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.39%. Comparing base (9d93b40) to head (2c8c58f).

❗ There is a different number of reports uploaded between BASE (9d93b40) and HEAD (2c8c58f). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (9d93b40) HEAD (2c8c58f)
tracer-php 12 11
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2896      +/-   ##
============================================
- Coverage     78.24%   72.39%   -5.85%     
  Complexity     2526     2526              
============================================
  Files           135      135              
  Lines         14400    14400              
  Branches        989      989              
============================================
- Hits          11267    10425     -842     
- Misses         2591     3433     +842     
  Partials        542      542              
Flag Coverage Δ
appsec-extension 68.36% <ø> (ø)
tracer-php 73.96% <ø> (-8.13%) ⬇️

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

see 8 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 9d93b40...2c8c58f. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Oct 16, 2024

Benchmarks [ tracer ]

Benchmark execution time: 2024-11-01 23:41:55

Comparing candidate commit 2c8c58f in PR branch khanayan123/implement-config-consistency-2 with baseline commit 9d93b40 in branch master.

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

scenario:PDOBench/benchPDOBaseline

  • 🟥 execution_time [+8.191µs; +13.468µs] or [+4.581%; +7.533%]

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟩 execution_time [-15.103µs; -12.634µs] or [-7.838%; -6.556%]

scenario:TraceFlushBench/benchFlushTrace

  • 🟩 execution_time [-1000.000ns; -1000.000ns] or [-50.000%; -50.000%]

ext/ddtrace.c Outdated
@@ -1096,6 +1096,10 @@ static zval *ddtrace_root_span_data_write(zend_object *object, zend_string *memb
} else if (zend_string_equals_literal(prop_name, "service")) {
if (ddtrace_span_is_entrypoint_root(&span->span) && !zend_is_identical(&span->property_service, value)) {
root_span_data_changed = true;
// As per unified service tagging spec if a span is created with a service name different from the global
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nearly, it needs to be changed when the span is not the root span.
And misses zval_ptr_dtor(&span->property_version); to first free to old value.

ext/ddtrace.c Outdated
@@ -1096,6 +1096,10 @@ static zval *ddtrace_root_span_data_write(zend_object *object, zend_string *memb
} else if (zend_string_equals_literal(prop_name, "service")) {
if (ddtrace_span_is_entrypoint_root(&span->span) && !zend_is_identical(&span->property_service, value)) {
root_span_data_changed = true;
// As per unified service tagging spec if a span is created with a service name different from the global
// service name it will not inherit the global version value
ZVAL_NULL(&span->property_version);
Copy link
Collaborator

@bwoebi bwoebi Oct 22, 2024

Choose a reason for hiding this comment

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

This cannot be null, use ZVAL_EMPTY_STRING instead. (see definition of SpanData class in ddtrace.stup.php)

@khanayan123 khanayan123 requested a review from bwoebi October 22, 2024 19:52
@khanayan123 khanayan123 requested a review from bwoebi October 30, 2024 16:56
@khanayan123 khanayan123 force-pushed the khanayan123/implement-config-consistency-2 branch from 7c66d29 to 0f2e7dc Compare November 1, 2024 23:12
@khanayan123 khanayan123 changed the title init Implement UST Consistency Nov 4, 2024
@khanayan123 khanayan123 marked this pull request as ready for review November 4, 2024 16:40
@khanayan123 khanayan123 requested a review from a team as a code owner November 4, 2024 16:40
@khanayan123 khanayan123 merged commit 352d3ba into master Nov 4, 2024
502 of 528 checks passed
@khanayan123 khanayan123 deleted the khanayan123/implement-config-consistency-2 branch November 4, 2024 16:48
@github-actions github-actions bot added this to the 1.5.0 milestone Nov 4, 2024
khanayan123 added a commit that referenced this pull request Nov 4, 2024
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.

3 participants