Move from uint63 to uint64 for trace_id span_id and parent_id #1237

labbati · 2021-05-18T12:49:24Z

Motivation

PHP has not a concept of uint64, only int64, equivalent to unint63 for this discussion. As a consequence, when a frontend non-PHP service generates a random uint64 as the trace/span ID, then this ID is wrongly converted to a uint63 (or int64) by PHP and propagated.

Description

Move from uint63 IDs to unit64. After this R:

In userland, IDs are always and only strings;
In C, IDs are only uint64;
Conversion from into uint64 is done as the last step during the array serialization into a msgpack structure;

Readiness checklist

(only for Members) Changelog has been added to the release document.
Tests added for this feature/bug.

Reviewer checklist

Appropriate labels assigned.
Milestone is set.
Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

labbati · 2021-05-18T13:03:16Z

@SammyK and @bwoebi I'd like to receive an opinion on the approach, so I am opening this PR as draft. In the description I summarized the motivation behind this and why we need this change.

…s a string

bwoebi · 2021-05-19T09:24:26Z

ext/php7/serializer.c

+            if (string_as_uint64) {
+                mpack_write_u64(writer, strtoull(ZSTR_VAL(Z_STR_P(trace)), NULL, 10));
+            } else {
+                mpack_write_cstr(writer, ZSTR_VAL(Z_STR_P(trace)));


Use the more idiomatic variant: Z_STRVAL_P(s) == ZSTR_VAL(Z_STR_P(s))

…zval

bwoebi · 2021-05-19T10:24:28Z

ext/php7/serializer.c

 ZEND_EXTERN_MODULE_GLOBALS(ddtrace);

-static int msgpack_write_zval(mpack_writer_t *writer, zval *trace TSRMLS_DC);
+static const char *trace_id_keyword = "trace_id";


Either use the variable everywhere (i.e. also in the add_assoc_string below) or not at all.

Very good point, did not catch that!

bwoebi

Looks good in general

SammyK

Nice work so far @labbati! I left some suggestions on the PHP 5 code that applies to 7 & 8 (with the exception of the add_assoc_string duplication issue which is PHP 5 specific).

ext/php5/random.c

ext/php5/serializer.c

tests/Common/TracerTestTrait.php

tests/ext/dd_trace_push_span_id_from_userland.phpt

labbati

Thank you for this first round of review @SammyK and @bwoebi

ext/php5/serializer.c

labbati · 2021-05-21T08:29:46Z

ext/php7/serializer.c

 ZEND_EXTERN_MODULE_GLOBALS(ddtrace);

-static int msgpack_write_zval(mpack_writer_t *writer, zval *trace TSRMLS_DC);
+static const char *trace_id_keyword = "trace_id";


Very good point, did not catch that!

Co-authored-by: Sammy Kaye Powers <sammyk@datadoghq.com>

… labbati/uint64

bwoebi · 2021-05-21T13:44:33Z

ext/php5/serializer.c

@@ -17,6 +19,11 @@

 ZEND_EXTERN_MODULE_GLOBALS(ddtrace);

+#define MAX_ID_LEN 20  // 1.8e^19 = 20 chars


TBH, this is not defensive, it's easy to forget a + 1 when allocating the buffer and it does no harm to have one byte too much allocated on the stack, better do 21 here and call it maybe MAX_ID_BUFSIZ

Very good point @bwoebi. I'll have to change my go-to way of doing this from now on. :)

Like it. Done

bwoebi · 2021-05-21T13:46:15Z

ext/php8/serializer.c

+        // Writing the value
+        if (zval_string_as_uint64) {
+            mpack_write_u64(writer, strtoull(Z_STRVAL_P(tmp), NULL, 10));
+        } else if (msgpack_write_zval(writer, tmp TSRMLS_CC) != 1) {


Don't add new TSRMLS - maybe rebase it, then compilation would fail (remove it in PHP 7 and PHP 8)

👍🏻 just pulled in master

…gent in integrations tests

…/uint64

bwoebi

Looks good now :-)

SammyK

Looks great, @labbati! 💯 I left one optional suggestion about future proofing how we're using sprintf (but this could be a separate PR). LGTM. 👍

ext/php5/serializer.c

…ts IDs

ext/php5/serializer.c

SammyK

Thanks for knocking out that last change! 👍

labbati · 2021-05-26T08:36:06Z

The two failing tests are unrelated to this PR (master fails as well) and we are going to fix the failures in a separate PR. Merging it right now.

labbati added this to the 0.60.0 milestone May 18, 2021

labbati added the c-extension Apply this label to issues and prs related to the C-extension label May 18, 2021

Base automatically changed from labbati/remove-json to master May 18, 2021 14:07

labbati added 8 commits May 18, 2021 16:14

migrate from u63 to u64 as trace/span ids

cca16ba

fix ext tests after moving from uint63 to uint64 passed to userland a…

bb06acc

…s a string

tmp

15f47f8

more changes toward uint64

7de7cb1

adapt tests to request-replayer that returns string for *_id fields

49a7dda

change assertion on IDs back from request replayer from int to str

5ddc8f0

use request replayer image that supports uint64 IDs

1681f37

use request replayer image that supports uint64 IDs (followup)

99dc2a6

bwoebi reviewed May 19, 2021

View reviewed changes

use the more ideomatic Z_STRVAL_P to access string value from string …

ddb8c46

…zval

labbati force-pushed the labbati/uint64 branch from 6ec4c4f to ddb8c46 Compare May 19, 2021 10:15

bwoebi reviewed May 19, 2021

View reviewed changes

bwoebi previously approved these changes May 19, 2021

View reviewed changes

move PHP8 to uint64 IDs

536b6ca

labbati dismissed bwoebi’s stale review via 536b6ca May 19, 2021 10:36

labbati added 3 commits May 19, 2021 12:39

move PHP5 to uint64 IDs

c5b58d3

remove ununsed import

d4ba19b

add test for istributed tracing with with trace/parent ids > PHP_INT_MAX

cd0babb

SammyK reviewed May 19, 2021

View reviewed changes

labbati commented May 21, 2021

View reviewed changes

labbati and others added 6 commits May 21, 2021 11:47

Apply suggestions from code review

98164ce

Co-authored-by: Sammy Kaye Powers <sammyk@datadoghq.com>

Merge branch 'labbati/uint64' of github.com:DataDog/dd-trace-php into…

2e7ef40

… labbati/uint64

improvements to php5 uint64 IDs handling

f0441cc

refactor serializer for uint64 IDs to improve readability

a710b3b

refactor serializer for uint64 IDs to improve readability (followup)

7a696b7

refactor serializer for uint64 IDs to improve readability (followup)

0981fc9

bwoebi reviewed May 21, 2021

View reviewed changes

labbati added 5 commits May 24, 2021 16:21

fix ID types while reconstructing the span structure as sent to the a…

47a249c

…gent in integrations tests

use constants in place of repeated strings in ext serializer

3c8eb3a

remove unused method from tests

c3e53a4

more defensive buf size to store IDs as string, including terminator

4f337fa

Merge branch 'master' of github.com:DataDog/dd-trace-php into labbati…

1bcc590

…/uint64

labbati marked this pull request as ready for review May 25, 2021 08:28

bwoebi previously approved these changes May 25, 2021

View reviewed changes

SammyK previously approved these changes May 25, 2021

View reviewed changes

ext/php5/serializer.c Outdated Show resolved Hide resolved

be more future proof using PRIu64 in place of %zu while writing 64 bi…

42d8cbc

…ts IDs

labbati dismissed stale reviews from SammyK and bwoebi via 42d8cbc May 25, 2021 14:22

SammyK reviewed May 25, 2021

View reviewed changes

ext/php5/serializer.c Show resolved Hide resolved

be more future proof using PRIu64 in place of %zu (followup)

765d602

SammyK approved these changes May 25, 2021

View reviewed changes

labbati merged commit bd6fd2f into master May 26, 2021

labbati deleted the labbati/uint64 branch May 26, 2021 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from uint63 to uint64 for trace_id span_id and parent_id #1237

Move from uint63 to uint64 for trace_id span_id and parent_id #1237

labbati commented May 18, 2021 •

edited

labbati commented May 18, 2021

bwoebi May 19, 2021

bwoebi May 19, 2021

labbati May 21, 2021

bwoebi left a comment

SammyK left a comment

labbati left a comment

labbati May 21, 2021

bwoebi May 21, 2021

SammyK May 21, 2021

labbati May 24, 2021

bwoebi May 21, 2021 •

edited

labbati May 24, 2021

bwoebi left a comment

SammyK left a comment

SammyK left a comment

labbati commented May 26, 2021

		@@ -17,6 +19,11 @@

		ZEND_EXTERN_MODULE_GLOBALS(ddtrace);

		#define MAX_ID_LEN 20 // 1.8e^19 = 20 chars

Move from uint63 to uint64 for trace_id span_id and parent_id #1237

Move from uint63 to uint64 for trace_id span_id and parent_id #1237

Conversation

labbati commented May 18, 2021 • edited

Motivation

Description

Readiness checklist

Reviewer checklist

labbati commented May 18, 2021

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

SammyK left a comment

Choose a reason for hiding this comment

labbati 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

bwoebi May 21, 2021 • edited

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

SammyK left a comment

Choose a reason for hiding this comment

SammyK left a comment

Choose a reason for hiding this comment

labbati commented May 26, 2021

labbati commented May 18, 2021 •

edited

bwoebi May 21, 2021 •

edited