Convert local root span id to u64, fix clippy lints #80

morrisonlevi · 2022-12-12T21:02:36Z

What does this PR do?

Fix clippy::uninlined_format_args lints.
Fix unused import std::path::Path in cfg(test).
Converts local_root_span_id in add_endpoint and related functions to be u64 instead of a string.
Removes unnecessary Cow<str> for str in label.
Bumps version to v2.0.0.

Motivation

The profiling backend can now accept numeric labels. This PR converts "local root span id" related parameters to use numbers in the endpoint API.

Additional Notes

This should have been multiple PRs. I'm sorry.

How to test the change?

Change add_endpoint to use u64 instead of a string.
Change your local root span id labels to use the .num variant instead of .str. Transmute from u64 to i64 as necessary. If they aren't changed, then profile.add will fail.

And then test as normal. Double-check that Code Hotspots and Endpoint Profiling both work.

Bump to v2.0.0

ivoanjo · 2022-12-13T09:06:05Z

profiling/src/profile/mod.rs

+        } else {
+            /* Sad path: have to fetch the string from the table and convert
+             * it back into a u64. Clients should change to use numbers!
+             */
+            let string = match self.strings.get_index(label.str as usize) {
+                None => anyhow::bail!(
+                    "failed to retrieve index {} from the string table when looking for endpoint data",
+                    label.str
+                ),
+                Some(string) => string,
+            };
+            string.as_str().parse()?
+        };


As this PR changes add_endpoint to take a u64 and not a String, I'm not quite sure we need this.

E.g., what clients would use the new numberic add_endpoint but still send the labels as strings elsewhere?

This is coming from a label, and there's nothing that stops a client from attaching the label value as a string instead of a number. So it seems prudent to have the path, although it shouldn't be used.

Hmm... I think having the fallback path is a sharper edge, as it makes things work even when they're not set up the way they should.

E.g., if someone does it wrong, they will still get the right output, which is dangerous...

One suggestion is to consider failing serialization and complaining that this label should not be a string. The serialize operation already returns a Result<EncodedProfile, EncodeError> so clients already need to handle errors, and perhaps modifying from to potentially return an error would allow us to report back what's wrong.

Okay, I've started working on fixing this. There will be two changes:

At serialization time, verify that any local root span id labels use .num.

When the sample is added, verify that any local root span id labels use .num.

profiling/src/profile/mod.rs

morrisonlevi · 2023-01-06T03:44:00Z

I confirmed that Code Hotspots and Endpoint profiling work for PHP with this branch.

**What does this PR do?**: In #73, we changed a number of types, functions and enums to be properly scoped inside or outside namespaces. But while integrating the 1.0.1 release in Ruby, I noticed that by mistake I forgot to properly configure the renaming for `Vec_U8`, which resulted in two copies of `Vec_U8` being emitted in the header files: `ddog_Vec_U8` and `ddof_prof_Vec_U8`, with some APIs using one and others using the other. This PR fixes the configuration. **Motivation**: Having two copies of the same struct is not correct and is really annoying for clients as well -- I temporarily needed to duplicate a helper on the Ruby profiler to have variants for both types (since often we don't even use a pointer but the struct directly, you can't even cast between them). **Additional Notes**: Here's the diff for the generated headers before/after this change: ```diff diff -uwr old-include/datadog/common.h build-linux/x86_64-linux/include/datadog/common.h --- old-include/datadog/common.h 2023-01-06 10:57:32.730644531 +0000 +++ build-linux/x86_64-linux/include/datadog/common.h 2023-01-06 10:58:41.954571430 +0000 @@ -106,16 +106,6 @@ */ typedef struct ddog_prof_Exporter_Request ddog_prof_Exporter_Request; -/** - * Holds the raw parts of a Rust Vec; it should only be created from Rust, - * never from C. - */ -typedef struct ddog_prof_Vec_U8 { - const uint8_t *ptr; - uintptr_t len; - uintptr_t capacity; -} ddog_prof_Vec_U8; - typedef enum ddog_prof_Exporter_NewResult_Tag { DDOG_PROF_EXPORTER_NEW_RESULT_OK, DDOG_PROF_EXPORTER_NEW_RESULT_ERR, @@ -128,7 +118,7 @@ struct ddog_prof_Exporter *ok; }; struct { - struct ddog_prof_Vec_U8 err; + struct ddog_Vec_U8 err; }; }; } ddog_prof_Exporter_NewResult; @@ -205,7 +195,7 @@ struct ddog_HttpStatus http_response; }; struct { - struct ddog_prof_Vec_U8 err; + struct ddog_Vec_U8 err; }; }; } ddog_prof_Exporter_SendResult; @@ -399,7 +389,7 @@ typedef struct ddog_prof_EncodedProfile { struct ddog_Timespec start; struct ddog_Timespec end; - struct ddog_prof_Vec_U8 buffer; + struct ddog_Vec_U8 buffer; struct ddog_prof_ProfiledEndpointsStats *endpoints_stats; } ddog_prof_EncodedProfile; @@ -415,7 +405,7 @@ struct ddog_prof_EncodedProfile ok; }; struct { - struct ddog_prof_Vec_U8 err; + struct ddog_Vec_U8 err; }; }; } ddog_prof_Profile_SerializeResult; diff -uwr old-include/datadog/profiling.h build-linux/x86_64-linux/include/datadog/profiling.h --- old-include/datadog/profiling.h 2023-01-06 10:57:32.730644531 +0000 +++ build-linux/x86_64-linux/include/datadog/profiling.h 2023-01-06 10:58:41.954571430 +0000 @@ -224,7 +224,7 @@ void ddog_prof_Profile_SerializeResult_drop(struct ddog_prof_Profile_SerializeResult _result); -DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_prof_Vec_U8 *vec); +DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_Vec_U8 *vec); /** * Resets all data in `profile` except the sample types and period. Returns ``` This is technically a breaking change for users of the C bindings, since a type gets removed (even if the type was never supposed to have existed). Since #80 is already bumping the libdatadog version to 2.0.0, I suggest only merging this PR after that one, and including it in the 2.0.0 release. **How to test the change?**: This change doesn't actually affect the compiled library, only the header files. Thus, the only way of noticing and testing this change, is to compile something that includes the headers and validate that only `ddog_Vec_U8` gets used.

…tring This is a newly-added feature to both the profiling backend, as well as libdatadog. Instead of sending the local root span ids as strings (that take up space in the string table, etc), we can now send them as numbers. We already did a similar migration for span ids in #2476, but at the time we did not change the local root span ids because the libdatadog `ddog_prof_Profile_set_endpoint` API could not properly handle the numeric ids (this has been changed in DataDog/libdatadog#80 ). As far as testing goes, I could have chosen to abstract away this change from our specs BUT I chose to make it explicit and changed the specs to match. I like having visibility in the specs of the values being encoded and passed around as numbers and not strings.

ivoanjo · 2023-01-06T15:20:12Z

🎉 I too was able to check that Code Hotspots and Endpoint profiling work fine for Ruby with this branch.

ivoanjo

Left a few notes! I ran out of time and didn't fully finish my review, will come back to it on monday :)

ivoanjo · 2023-01-06T16:20:09Z

ddtelemetry/src/config.rs

        } else {
-            format!("{}{}", &agent_url, AGENT_TELEMETRY_URL_PATH)
+            format!("{}{AGENT_TELEMETRY_URL_PATH}", &agent_url)


Interesting -- is there no extended way for format! that can embed &agent_url directly?

No, the inline format values can only contain simple identifiers https://rust-lang.github.io/rfcs/2795-format-args-implicit-identifiers.html

For the sake of uniformity, maybe named arguments would be better

Suggested change

format!("{}{AGENT_TELEMETRY_URL_PATH}", &agent_url)

format!("{agent_url}{AGENT_TELEMETRY_URL_PATH}", agent_url=&agent_url)

ivoanjo · 2023-01-06T16:21:06Z

profiling-ffi/src/profiles.rs

-/// * `local_root_span_id` - the value of the local root span id label to look for.
+/// * `local_root_span_id`


Minor: This is a weird change -- was it intentional?

ivoanjo · 2023-01-06T16:26:30Z

profiling/src/profile/mod.rs

+            labels.push(Label {
+                key,
+                str,
+                num: label.num,
+                num_unit,
+            });


Lol is it me or is this mix of positional and keyword arguments a bit weird? 🤔

It doesn't bother me 🤷🏻

profiling/src/profile/mod.rs

This avoid doing runtime checking and simplifies some logic.

ivoanjo

👍 LGTM, thanks for this tiny but really needed improvement :)

profiling/src/profile/mod.rs

**What does this PR do?**: In #73, we changed a number of types, functions and enums to be properly scoped inside or outside namespaces. But while integrating the 1.0.1 release in Ruby, I noticed that by mistake I forgot to properly configure the renaming for `Vec_U8`, which resulted in two copies of `Vec_U8` being emitted in the header files: `ddog_Vec_U8` and `ddof_prof_Vec_U8`, with some APIs using one and others using the other. This PR fixes the configuration. **Motivation**: Having two copies of the same struct is not correct and is really annoying for clients as well -- I temporarily needed to duplicate a helper on the Ruby profiler to have variants for both types (since often we don't even use a pointer but the struct directly, you can't even cast between them). **Additional Notes**: Here's the diff for the generated headers before/after this change: ```diff diff -uwr old-include/datadog/common.h build-linux/x86_64-linux/include/datadog/common.h --- old-include/datadog/common.h 2023-01-06 10:57:32.730644531 +0000 +++ build-linux/x86_64-linux/include/datadog/common.h 2023-01-06 10:58:41.954571430 +0000 @@ -106,16 +106,6 @@ */ typedef struct ddog_prof_Exporter_Request ddog_prof_Exporter_Request; -/** - * Holds the raw parts of a Rust Vec; it should only be created from Rust, - * never from C. - */ -typedef struct ddog_prof_Vec_U8 { - const uint8_t *ptr; - uintptr_t len; - uintptr_t capacity; -} ddog_prof_Vec_U8; - typedef enum ddog_prof_Exporter_NewResult_Tag { DDOG_PROF_EXPORTER_NEW_RESULT_OK, DDOG_PROF_EXPORTER_NEW_RESULT_ERR, @@ -128,7 +118,7 @@ struct ddog_prof_Exporter *ok; }; struct { - struct ddog_prof_Vec_U8 err; + struct ddog_Vec_U8 err; }; }; } ddog_prof_Exporter_NewResult; @@ -205,7 +195,7 @@ struct ddog_HttpStatus http_response; }; struct { - struct ddog_prof_Vec_U8 err; + struct ddog_Vec_U8 err; }; }; } ddog_prof_Exporter_SendResult; @@ -399,7 +389,7 @@ typedef struct ddog_prof_EncodedProfile { struct ddog_Timespec start; struct ddog_Timespec end; - struct ddog_prof_Vec_U8 buffer; + struct ddog_Vec_U8 buffer; struct ddog_prof_ProfiledEndpointsStats *endpoints_stats; } ddog_prof_EncodedProfile; @@ -415,7 +405,7 @@ struct ddog_prof_EncodedProfile ok; }; struct { - struct ddog_prof_Vec_U8 err; + struct ddog_Vec_U8 err; }; }; } ddog_prof_Profile_SerializeResult; diff -uwr old-include/datadog/profiling.h build-linux/x86_64-linux/include/datadog/profiling.h --- old-include/datadog/profiling.h 2023-01-06 10:57:32.730644531 +0000 +++ build-linux/x86_64-linux/include/datadog/profiling.h 2023-01-06 10:58:41.954571430 +0000 @@ -224,7 +224,7 @@ void ddog_prof_Profile_SerializeResult_drop(struct ddog_prof_Profile_SerializeResult _result); -DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_prof_Vec_U8 *vec); +DDOG_CHECK_RETURN struct ddog_Slice_U8 ddog_Vec_U8_as_slice(const struct ddog_Vec_U8 *vec); /** * Resets all data in `profile` except the sample types and period. Returns ``` This is technically a breaking change for users of the C bindings, since a type gets removed (even if the type was never supposed to have existed). Since #80 is already bumping the libdatadog version to 2.0.0, I suggest only merging this PR after that one, and including it in the 2.0.0 release. **How to test the change?**: This change doesn't actually affect the compiled library, only the header files. Thus, the only way of noticing and testing this change, is to compile something that includes the headers and validate that only `ddog_Vec_U8` gets used. Co-authored-by: Levi Morrison <levi.morrison@datadoghq.com>

**What does this PR do?**: Documents the expected result of the `ddog_prof_Profile_add` API. **Motivation**: While validating #80 on Ruby, I called `add` in a way that should've triggered an error, but saw none. On closer inspection, I had no error handling for `add` because it wasn't clear to me that its result could indicate an error. Thus, I decided to document that fact. **Additional Notes**: At some point we could change `add` to return an actual result, allowing us to pass the error message back to the caller, but for now I decided to just document it. **How to test the change?**: Documentation change only.

…tring This is a newly-added feature to both the profiling backend, as well as libdatadog. Instead of sending the local root span ids as strings (that take up space in the string table, etc), we can now send them as numbers. We already did a similar migration for span ids in #2476, but at the time we did not change the local root span ids because the libdatadog `ddog_prof_Profile_set_endpoint` API could not properly handle the numeric ids (this has been changed in DataDog/libdatadog#80 ). As far as testing goes, I could have chosen to abstract away this change from our specs BUT I chose to make it explicit and changed the specs to match. I like having visibility in the specs of the values being encoded and passed around as numbers and not strings.

morrisonlevi added 2 commits December 12, 2022 13:54

Convert local root span id to u64, fix clippy lints

4990cc4

Bump to v2.0.0

Fix path

85cdec2

morrisonlevi mentioned this pull request Dec 12, 2022

perf: use numbers instead of numeric strings in pprof labels DataDog/dd-trace-php#1831

Merged

4 tasks

ivoanjo reviewed Dec 13, 2022

View reviewed changes

add test for root_span_id label as str or i64

e391261

morrisonlevi commented Jan 3, 2023

View reviewed changes

profiling/src/profile/mod.rs Show resolved Hide resolved

morrisonlevi force-pushed the levi/endpoint-num branch from a850af5 to e391261 Compare January 4, 2023 22:35

morrisonlevi added 4 commits January 4, 2023 15:59

Test trace endpoint labels too

3eb917a

Ensure local root span id uses .num label value

0a83855

Fix clippy::manual-map lint

d6ab0bd

Fix clippy::needless_question_mark lint

344b3e3

ivoanjo mentioned this pull request Jan 6, 2023

Fix C headers including both ddog_Vec_U8 and ddog_prof_Vec_U8 #83

Merged

ivoanjo reviewed Jan 6, 2023

View reviewed changes

morrisonlevi added 2 commits January 6, 2023 14:09

Eagerly store certain strings

e131455

This avoid doing runtime checking and simplifies some logic.

Merge branch 'main' into levi/endpoint-num

2d662d4

morrisonlevi marked this pull request as ready for review January 7, 2023 22:20

morrisonlevi requested review from a team as code owners January 7, 2023 22:20

ivoanjo approved these changes Jan 9, 2023

View reviewed changes

profiling/src/profile/mod.rs Show resolved Hide resolved

profiling/src/profile/mod.rs Show resolved Hide resolved

profiling/src/profile/mod.rs Show resolved Hide resolved

Test span ids larger than i64::MAX

1d3096e

morrisonlevi merged commit 2ed9132 into main Jan 11, 2023

morrisonlevi deleted the levi/endpoint-num branch January 11, 2023 16:59

ivoanjo mentioned this pull request Jan 13, 2023

Minor: Document expected result of ddog_prof_Profile_add #87

Merged

morrisonlevi mentioned this pull request Jan 17, 2023

fix(profiling)!: ensure a single label per sample has key "local root span id" #88

Merged

morrisonlevi added the profiling Relates to the profiling* modules. label Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert local root span id to u64, fix clippy lints #80

Convert local root span id to u64, fix clippy lints #80

morrisonlevi commented Dec 12, 2022 •

edited

Loading

ivoanjo Dec 13, 2022

morrisonlevi Dec 16, 2022 •

edited

Loading

ivoanjo Jan 4, 2023

morrisonlevi Jan 5, 2023

morrisonlevi commented Jan 6, 2023

ivoanjo commented Jan 6, 2023

ivoanjo left a comment

ivoanjo Jan 6, 2023

paullegranddc Jan 9, 2023

ivoanjo Jan 6, 2023

ivoanjo Jan 6, 2023

morrisonlevi Jan 6, 2023

ivoanjo left a comment

	format!("{}{AGENT_TELEMETRY_URL_PATH}", &agent_url)
	format!("{agent_url}{AGENT_TELEMETRY_URL_PATH}", agent_url=&agent_url)

		/// * `local_root_span_id` - the value of the local root span id label to look for.
		/// * `local_root_span_id`

Convert local root span id to u64, fix clippy lints #80

Convert local root span id to u64, fix clippy lints #80

Conversation

morrisonlevi commented Dec 12, 2022 • edited Loading

What does this PR do?

Motivation

Additional Notes

How to test the change?

Choose a reason for hiding this comment

morrisonlevi Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morrisonlevi commented Jan 6, 2023

ivoanjo commented Jan 6, 2023

ivoanjo 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

ivoanjo left a comment

Choose a reason for hiding this comment

morrisonlevi commented Dec 12, 2022 •

edited

Loading

morrisonlevi Dec 16, 2022 •

edited

Loading