Create a span normalizer skeleton, fully implement span name normalization by thedavl · Pull Request #100 · DataDog/libdatadog

thedavl · 2023-01-26T18:41:42Z

What does this PR do?

This PR adds a span normalizer skeleton (riddled with TODOs), but includes fully implemented span name normalization.

Future PRs (addressing TODOs) will complete:

span tag normalization + unit tests
span service normalization + unit tests.
normalizer-wide unit tests
trace normalization + unit tests

The span normalizer (as well as future functions completing TODOs) is modeled very closely after the existing agent trace normalizer, which can be found here.

Note: the unit tests are translated directly from the agent normalizer.

Motivation

The trace normalizer will be used in our serverless agentless POC, which will use it to normalize traces before being sent directly to the datadog trace intake.

See: the serverless branch

Additional Notes

agent/tracer/span proto files were copied from https://github.com/DataDog/datadog-agent/tree/main/pkg/trace/pb

How to test the change?

The changes have been manually tested using the agentless POC, normalizing span names and verifying the normalization works as expected. Unit tests have been copied/translated from the agent normalizer unit tests written in go.

thedavl · 2023-01-26T20:11:12Z

+const YEAR_2000_NANOSEC_TS: i64 = 946684800000000000;
+
+#[allow(dead_code)]
+fn normalize(s: &mut pb::Span) -> Result<(), errors::NormalizerError> {


span normalizer which will be used by future normalize_trace function.

morrisonlevi

This wasn't a thorough review, just a drive-by to get started. Will check more later, probably tomorrow!

- moved everything from trace/ to trace-normalization/ - removed all print statements - renamed package - change the type of all size constants to usize

morrisonlevi · 2023-01-30T21:43:26Z

+pub fn normalize(s: &mut pb::Span) -> Result<(), errors::NormalizerError> {
+    if s.trace_id == 0 {
+        return Err(errors::NormalizerError::new("TraceID is zero (reason:trace_id_zero)"));
+    }
+    if s.span_id == 0 {
+        return Err(errors::NormalizerError::new("SpanID is zero (reason:span_id_zero)"));
+    }


I'm not sure if anyone on your team has opinions, but I would recommend pulling in the anyhow crate instead of rolling your own errors. It has some nice macros to simplify code around error paths. For instance, the anyhow::ensure! macro would reduce the code to something like this:

pub fn normalize(s: &mut pb::Span) -> anyhow::Result<()> { anyhow::ensure!(s.trace_id != 0, "TraceID is zero (reason:trace_id_zero)"); anyhow::ensure!(s.span_id != 0, "SpanID is zero (reason:span_id_zero)");

this makes sense, I'll make these changes

@morrisonlevi I think if other crates in the repo are using Anyhow, we can follow suit. I noticed the Vector team uses Snafu, was that ever considered for libdatadog.

@DarcyRaynerDD I hadn't heard of Snafu until you mention it, so that pretty much sums up why it wasn't considered :)

DarcyRaynerDD

Left a few nits. Overall excellent work.

DarcyRaynerDD · 2023-02-01T15:52:52Z

+pub fn normalize(s: &mut pb::Span) -> Result<(), errors::NormalizerError> {
+    if s.trace_id == 0 {
+        return Err(errors::NormalizerError::new("TraceID is zero (reason:trace_id_zero)"));
+    }
+    if s.span_id == 0 {
+        return Err(errors::NormalizerError::new("SpanID is zero (reason:span_id_zero)"));
+    }


@morrisonlevi I think if other crates in the repo are using Anyhow, we can follow suit. I noticed the Vector team uses Snafu, was that ever considered for libdatadog.

- remove [workspace] from trace-normalization/cargo.toml - comment typos - remove use of unwrap()

Co-authored-by: Levi Morrison <levi.morrison@datadoghq.com>

commit 72af2a0 Author: David Lee <thedavl2001@gmail.com> Date: Mon Feb 13 15:44:57 2023 -0800 Trace Normalizer: add trace & trace chunk normalization (#109) commit 510472d Author: Pawel Chojnacki <pawelchcki@gmail.com> Date: Thu Feb 9 15:05:33 2023 +0100 Fixup build-telemetry-ffi and cbindgen.toml (#105) commit 711bd43 Author: David Lee <thedavl2001@gmail.com> Date: Wed Feb 8 15:18:24 2023 -0800 Trace Normalizer: add service + env tag normalization (#106) * add service tag + env tag normalization + unit tests commit ee4bba8 Author: Ivo Anjo <ivo.anjo@datadoghq.com> Date: Tue Feb 7 15:38:13 2023 +0000 Remove unneeded "gem signout" step from Ruby release (#108) **What does this PR do?**: This PR removes the now-unneeded "gem signout" steps during the Ruby release process. **Motivation**: In #85, we changed the way that we authenticate with rubygems.org when pushing a new libdatadog release. I just did a release with this new code, and noticed that because we no longer log in, but just use a limited API key, the "gem signout" does not do anything and emits an error. Here's what I saw when I ran `docker-compose run push_to_rubygems`: ``` ... preparation of packages goes here... ERROR: You are not currently signed in. Please input 'libdatadog ruby release key' from 'Profiling - Falcon' Datadog 1Password: (...key...) Pushing gem to https://rubygems.org... You have enabled multi-factor authentication. Please enter OTP code. Code: (...) Successfully registered gem: libdatadog (2.0.0.1.0) Pushing gem to https://rubygems.org... You have enabled multi-factor authentication. Please enter OTP code. Code: (...) Successfully registered gem: libdatadog (2.0.0.1.0-x86_64-linux) Pushing gem to https://rubygems.org... You have enabled multi-factor authentication. Please enter OTP code. Code: (...) Successfully registered gem: libdatadog (2.0.0.1.0-aarch64-linux) ERROR: You are not currently signed in. ``` Those two "ERROR: You are not currently signed in" come from the "gem signout" steps, and what's why I'm removing them. **Additional Notes**: (N/A) **How to test the change?**: You can run `docker-compose run push_to_rubygems` and validate the errors will not show up again. This is safe because Rubygems does not allow re-releasing the same packages. commit 8be5465 Author: Ivo Anjo <ivo.anjo@datadoghq.com> Date: Fri Feb 3 14:35:58 2023 +0000 Package libdatadog v2.0.0 for Ruby (#107) **What does this PR do?**: This PR includes the changes documented in the "Releasing a new version to rubygems.org" part of the README: <https://github.com/DataDog/libdatadog/tree/main/ruby#releasing-a-new-version-to-rubygemsorg> **Motivation**: Enable Ruby to use libdatadog v2.0.0. **Additional Notes**: (N/A) **How to test the change?**: This was locally checked with the Ruby profiler branch that already supports libdatadog 2. commit 73b8e2e Author: David Lee <thedavl2001@gmail.com> Date: Thu Feb 2 14:01:07 2023 -0800 Create a span normalizer skeleton, fully implement span name normalization (#100) commit 097ea5d Author: Levi Morrison <levi.morrison@datadoghq.com> Date: Thu Feb 2 10:32:39 2023 -0700 refactor(profiling)!: less chance of request double free (#103) commit e3887c3 Author: Pawel Chojnacki <pawelchcki@gmail.com> Date: Mon Jan 30 18:06:46 2023 +0100 Fix CI warnings (#104) * Fix warnings * clippy fix commit 19b7f69 Author: Levi Morrison <levi.morrison@datadoghq.com> Date: Fri Jan 27 10:34:37 2023 -0700 refactor(profiling)!: create FFI Error type and remove `*Result_drop` methods (#95) * refactor(profiling)!: create FFI Error type This extracts an FFI Error type `ddog_Error`. It contains an FFI Vec internally: ```rust /// Please treat this as opaque; do not reach into it, and especially /// don't write into it! pub struct Error { /// This is a String stuffed into the vec. message: Vec<u8>, } ``` FFI result types have been updated e.g.: ```rust pub enum SendResult { HttpResponse(HttpStatus), Err(Error), } ``` Instead of reaching into the buffer, use these APIs: ```c /** * # Safety * Only pass null or a valid reference to an Error. */ void ddog_Error_drop(struct ddog_Error *error); /** * Returns a CharSlice of the error's message that is valid until the error * is dropped. * # Safety * Only pass null or a valid reference to an Error. */ ddog_CharSlice ddog_Error_message(const struct ddog_Error *error); ``` * Drop *Result_drop functions The `*Result_drop` methods have been removed: - ddog_prof_Exporter_NewResult_drop - ddog_prof_Exporter_Request_BuildResult_drop - ddog_prof_Exporter_SendResult_drop - ddog_prof_Profile_AddResult_drop - ddog_prof_Profile_SerializeResult_drop - ddog_Vec_Tag_PushResult_drop And these were added instead: - ddog_Error_drop (+ ddog_Error_message to get the message) - ddog_prof_EncodedProfile_drop - ddog_prof_Exporter_Request_drop * Make a note about #[must_use] * Add ddog_prof_EncodedProfile_drop Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com> commit ed1ee92 Author: Florian Engelhardt <florian.engelhardt@datadoghq.com> Date: Fri Jan 27 17:19:30 2023 +0100 fix link to contribution guide (#102) commit 1c445fe Author: Levi Morrison <levi.morrison@datadoghq.com> Date: Fri Jan 27 09:02:54 2023 -0700 feat(build-profiling-ffi.sh): support CARGO_TARGET_DIR (#101) commit 88899ba Author: Levi Morrison <levi.morrison@datadoghq.com> Date: Fri Jan 27 08:43:11 2023 -0700 fix: clippy lints from Rust v1.67.0 release (#99) * fix clippy::uninlined_format_args * fix clippy::seek_to_start_instead_of_rewind commit f2d0ed0 Author: Levi Morrison <levi.morrison@datadoghq.com> Date: Wed Jan 25 15:32:33 2023 -0700 feat(profiling)!: pass errors through more FFI functions (#90) This changes the return types for FFI functions: - `ddog_prof_Profile_add` - `ddog_prof_Exporter_Request_build` Adds new structs: - `ddog_prof_Profile_AddResult` - `ddog_prof_Exporter_Request_BuildResult` And adds functions to drop them: - `ddog_prof_Profile_AddResult_drop` - `ddog_prof_Exporter_Request_BuildResult_drop` Removes a now-unnecessary newtype definition of struct `Request(exporter::Request)` as cbindgen handles the refactored code. commit c93b7c1 Author: Levi Morrison <levi.morrison@datadoghq.com> Date: Mon Jan 23 10:56:21 2023 -0700 chore: update dependencies (#96) * chore: update dependencies This fixes a dependabot alert: https://github.com/DataDog/libdatadog/security/dependabot/6 With the updated dependencies, there was a new deprecation: > warning: use of deprecated associated function > `chrono::TimeZone::timestamp`: use `timestamp_opt()` instead I replaced it with `timestamp_opt` and unwrapped it, which is exactly what the now-deprecated `timestamp` function does: https://github.com/chronotope/chrono/blob/378efb157d674c01761f538d4450705c2b1766a4/src/offset/mod.rs#L343 They deprecated it because they are working to not panic internally. * Bump LICENSE-3rdparty.yml Merge branch 'main' into origin/david.lee/agentless-use-trace-normalizer

thedavl added 9 commits January 26, 2023 10:05

span normilization

9ee0e0d

relocate normalization files

7dcc7d4

move normalization files into trace/

0cc2fcd

add proto files + lib.rs and lint

6ae7ff0

add license headers and lint

6f642c4

clippy allow dead code, removed unused dependencies in cargo.toml

77bb03d

remove trace from root dir cargo.toml workspaces

a87dcfa

Change code TODO comments

48b15ca

comment typo

ea7c455

thedavl marked this pull request as ready for review January 26, 2023 20:08

thedavl requested review from a team as code owners January 26, 2023 20:08

thedavl changed the title ~~Create a span normalizer, fully implement span name normalization~~ Create a span normalizer skeleton, fully implement span name normalization Jan 26, 2023

thedavl commented Jan 26, 2023

View reviewed changes

thedavl requested a review from IvanTopolcic January 26, 2023 20:18

morrisonlevi reviewed Jan 26, 2023

View reviewed changes

Comment thread trace/Cargo.toml Outdated

Comment thread trace/src/normalization/normalize_utils.rs Outdated

Comment thread trace/src/normalization/normalizer.rs Outdated

thedavl and others added 2 commits January 26, 2023 13:50

address PR comments

3916a97

- moved everything from trace/ to trace-normalization/ - removed all print statements - renamed package - change the type of all size constants to usize

Merge branch 'main' into david.lee/trace-normalize-metric-names

178271f

maxday reviewed Jan 30, 2023

View reviewed changes

Comment thread trace-normalization/src/normalization/normalize_utils.rs Outdated

maxday reviewed Jan 30, 2023

View reviewed changes

Comment thread trace-normalization/src/normalization/normalize_utils.rs Outdated

maxday reviewed Jan 30, 2023

View reviewed changes

Comment thread trace-normalization/src/normalization/normalizer.rs Outdated

thedavl and others added 3 commits January 30, 2023 11:44

address PR comments

e65ab7f

Merge branch 'main' into david.lee/trace-normalize-metric-names

493464e

refactor normalize_name to use Result<>

6815691

morrisonlevi reviewed Jan 30, 2023

View reviewed changes

Comment thread trace-normalization/src/pb.rs

Comment thread trace-normalization/src/normalization/normalizer.rs Outdated

fix strange indent

f7ab960

morrisonlevi reviewed Jan 30, 2023

View reviewed changes

thedavl added 2 commits January 30, 2023 13:59

fix more strange indents

e389a47

address PR comments. add package.json + index.js for napi-rs building

eb1e08b

DarcyRaynerDD reviewed Feb 1, 2023

View reviewed changes

thedavl and others added 9 commits February 1, 2023 12:41

reorganize directory structure (move tests into the files they test)

7c40f29

address PR comments

3a308bc

- remove [workspace] from trace-normalization/cargo.toml - comment typos - remove use of unwrap()

lint

1038086

use the duplicate crate for normalize_utils unit test

7a949ad

fix unit test

eb31f52

Update Cargo.lock

b4dc2ce

lint

f8b0e08

Address PR comments

5212dd4

Merge branch 'main' into david.lee/trace-normalize-metric-names

ae4653f

morrisonlevi reviewed Feb 2, 2023

View reviewed changes

Comment thread trace-normalization/Cargo.toml Outdated

morrisonlevi reviewed Feb 2, 2023

View reviewed changes

Comment thread trace-normalization/src/normalize_utils.rs Outdated

thedavl and others added 7 commits February 2, 2023 09:45

removed prost-build dep

99ad955

fix skip invalid name prefix bug, update tests/error messages.

c7496ba

change cargo.toml version (so it's the same as other libdd packages)

00e77b0

Co-authored-by: Levi Morrison <levi.morrison@datadoghq.com>

lint

c74f76e

clean up comments

f63cf45

reduce the amount of clones, fix unit tests, update lock file

1a3ac5d

lint

90797c8

morrisonlevi approved these changes Feb 2, 2023

View reviewed changes

morrisonlevi added the serverless label Feb 2, 2023

thedavl merged commit 73b8e2e into main Feb 2, 2023

thedavl deleted the david.lee/trace-normalize-metric-names branch February 2, 2023 22:01

Conversation

thedavl commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

How to test the change?

Uh oh!

thedavl Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morrisonlevi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

morrisonlevi Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

thedavl Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

DarcyRaynerDD Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

morrisonlevi Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DarcyRaynerDD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DarcyRaynerDD Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thedavl commented Jan 26, 2023 •

edited

Loading

thedavl Jan 26, 2023 •

edited

Loading