Minor adjustments to the data-pipeline/TraceExporter #388

ekump · 2024-04-11T19:40:38Z

What does this PR do?

Just some minor changes to the TraceExporter.

Added some tests around the From trait implementations for TracerTags. These were already tested indirectly.
Tweaked the unit tests to test the end result of calling the builder rather than the values of the builder struct fields.
Changed no_proxy to use_proxy to be (possibly) clearer and added necessary default trait impl for the builder.
Made the TracerTag fields on TraceExporterBuilder non-optional since they weren't actually optional.

Motivation

I incorrectly started to work on TraceExporter for APMSP-1020 before I was informed we should be implementing that ticket in the sidecar's trace export logic for now. (We'll address implementing in TraceExporter later).

Additional Notes

Anything else we should know when reviewing?

How to test the change?

Unit tests are in place

For Reviewers

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

ekump · 2024-04-11T19:47:01Z

data-pipeline/src/trace_exporter.rs

-        assert_eq!(builder.language_version.unwrap(), "1.0");
-        assert_eq!(builder.interpreter.unwrap(), "v8");
+
+        assert_eq!(exporter.tags.tracer_version, "v0.1");


I switched to testing the exporter tags because this test is calling build() which sets the exporter tags and we should be testing outputs in unit tests, unless we have a special reason not to.

ekump · 2024-04-11T19:51:16Z

data-pipeline/src/trace_exporter.rs

@@ -157,10 +157,10 @@ impl TraceExporter {
 pub struct TraceExporterBuilder {
    host: Option<String>,
    port: Option<u16>,
-    tracer_version: Option<String>,


The unwrapping of the tag values in the build function and the fact that the corresponding TracerTag values are not optional implies that they aren't actually optional. I switched them to String since that better signals intent and also leads to a very minor perf improvement in the builder (not a primary concern).

If there are future plans that rely on these values truly being options I can change them back.

ekump · 2024-04-11T19:52:28Z

data-pipeline/src/trace_exporter.rs

@@ -176,25 +176,25 @@ impl TraceExporterBuilder {
    }

    pub fn set_tracer_version(&mut self, tracer_version: &str) -> &mut TraceExporterBuilder {
-        self.tracer_version = Some(String::from(tracer_version));
+        self.tracer_version = tracer_version.to_owned();


I've always known to_owned() to be idiomatic for going from &str to String. Happy to revert this if it was done intentionally.

It was done intentionally. The reasoning behind it is, at least in this case, the builder won't cross multiple function boundaries. It's called from an init function and it's never reused so it doesn't need to own all the resources. Therefore cloning all the values and then dropping them at the end of the function didn't seem a good choice in terms of memory management. If you foresee any use case in which the builder has to own all the resources go for it if not it's probably better to not allocate all that strings but that's my opinion.

Nowadays, String::From and to_owned() are functionally equivalent, in both cases you're creating a String that the Builder owns. From a style perspective I personally prefer to_owned() as it looks cleaner (to me) and conveys intention of creating something the builder will own.

If the intention is that the builder should only hold the references, then I can change it to do that. But, is that something we really want to do? Because at the end of the day we probably want TracerTags to not hold references as they will likely get used in another thread and the function that calls the builder may drop the references before we're done with them?

We could keep TracerTags members as String, but have the Builder use &str and do the clone in the build() function to defer the memory allocation as long as possible if we think it's worth it.

You're right, we might changed it along the way, I'd need to review the commit history to remember why but I think in the beginning we were holding the references. Now it makes sense to me :).
The initial intention was to minimize memory allocations as much as possible but you solved that by using mem::take later in the build method.
Regarding to_owned vs from, I don't have any clear preference so if it works better for you we can leave that way and favor that option.

I'm in favor of to_owned as well.

ekump · 2024-04-11T20:05:33Z

data-pipeline/src/trace_exporter.rs

-                language_version: self.language_version.clone().unwrap(),
-                language_interpreter: self.interpreter.clone().unwrap(),
-                language: self.language.clone().unwrap(),
+                tracer_version: std::mem::take(&mut self.tracer_version),


I haven't run benchmarks, but using mem::take is slightly more performant versus to_owned() because to_owned() does another memory allocation. mem::take just swaps the existing value with the default. The downside to this is that the builder doesn't retain values after build() is called and thus can cause unexpected results if it is reused. I don't think I've ever reused a builder after I finished building, so I don't think it should be a problem? But, I'm happy to change this to to_owned() if people disagree.

If the build function is destructive in that way, then it should take ownership of the builder with fn build(self) ..., to avoid misuse.

It's fine to also just add a comment for now, and I'll change things when aligning the API with the RFC doc.

Good call. I updated the builder.

data-pipeline/src/trace_exporter.rs

ekump · 2024-04-12T19:00:44Z

data-pipeline/src/trace_exporter.rs

+    use_proxy: bool,
+}
+
+impl Default for TraceExporterBuilder {


I'm split on whether or not this is a good idea. On the one hand, we should avoid negative bool variables like no_proxy when possible as it can easily lead to confusion versus bools that are affirmatively named. On the other hand, I see why no_proxy was probably chosen in the first place since we want the default behavior to use the proxy and bools default to false naturally. Instead of using derive Default macro we had to implement it manually which can seem like overkill for just one struct field.

I can see an argument that this change makes the code more complicated in its attempt to make it simpler so I can revert back to no_proxy if anyone feels strongly.

non-optional Remove optionality of TracerTag related fields as they aren't optional. The build method was just unwrapping them, implying that they had to be set. Build currently uses mem::take for these values, which makes the builder not reusable.

affirmatively named booleans are less confusing to work with.

…now destructive

github-actions bot added the data-pipeline label Apr 11, 2024

ekump force-pushed the ekump/APMSP-1020-exporter-retry-strat branch from d7e101e to 739b50e Compare April 11, 2024 19:41

ekump commented Apr 11, 2024

View reviewed changes

ekump changed the title ~~Add retry logic to TraceExporter~~ WIP: Add retry logic to TraceExporter Apr 11, 2024

ekump commented Apr 11, 2024

View reviewed changes

data-pipeline/src/trace_exporter.rs Outdated Show resolved Hide resolved

ekump force-pushed the ekump/APMSP-1020-exporter-retry-strat branch 2 times, most recently from 4a55639 to 89758f4 Compare April 12, 2024 18:54

ekump commented Apr 12, 2024

View reviewed changes

ekump force-pushed the ekump/APMSP-1020-exporter-retry-strat branch from 89758f4 to 66f60f3 Compare April 12, 2024 19:18

ekump marked this pull request as ready for review April 12, 2024 19:19

ekump requested a review from a team as a code owner April 12, 2024 19:19

ekump changed the title ~~WIP: Add retry logic to TraceExporter~~ Add retry logic to TraceExporter Apr 12, 2024

ekump changed the title ~~Add retry logic to TraceExporter~~ Minor adjustments to the data-pipeline/TraceExporter Apr 15, 2024

bantonsson approved these changes Apr 15, 2024

View reviewed changes

ekump force-pushed the ekump/APMSP-1020-exporter-retry-strat branch 3 times, most recently from 90a24df to 4b5b7db Compare April 15, 2024 15:14

hoolioh approved these changes Apr 16, 2024

View reviewed changes

ekump added 4 commits April 17, 2024 08:15

implement explicit tests for From trait impls for TracerTags

4329955

change no_proxy field to use_proxy field in TraceExporter and builder

f659eff

affirmatively named booleans are less confusing to work with.

refactor TraceExportBuilder to take ownership of Builder since it is …

d32aa2f

…now destructive

ekump force-pushed the ekump/APMSP-1020-exporter-retry-strat branch from 4b5b7db to d32aa2f Compare April 17, 2024 12:15

ekump merged commit f7dadca into main Apr 17, 2024
20 checks passed

ekump deleted the ekump/APMSP-1020-exporter-retry-strat branch April 17, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor adjustments to the data-pipeline/TraceExporter #388

Minor adjustments to the data-pipeline/TraceExporter #388

ekump commented Apr 11, 2024 •

edited

Loading

ekump Apr 11, 2024

ekump Apr 11, 2024

ekump Apr 11, 2024 •

edited

Loading

hoolioh Apr 15, 2024

ekump Apr 15, 2024

hoolioh Apr 15, 2024

bantonsson Apr 15, 2024

ekump Apr 11, 2024

bantonsson Apr 15, 2024

bantonsson Apr 15, 2024

ekump Apr 15, 2024

ekump Apr 12, 2024

Minor adjustments to the data-pipeline/TraceExporter #388

Minor adjustments to the data-pipeline/TraceExporter #388

Conversation

ekump commented Apr 11, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

How to test the change?

For Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekump Apr 11, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekump commented Apr 11, 2024 •

edited

Loading

ekump Apr 11, 2024 •

edited

Loading