Paullgdc/telemetry ffi again #54

paullegranddc · 2022-09-23T16:33:57Z

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

How to test the change?

Describe here in detail how the change can be validated.

morrisonlevi · 2022-09-26T14:30:23Z

ddtelemetry-ffi/Cargo.toml

@@ -9,9 +9,10 @@ edition = "2021"
 [lib]
 # LTO is ignored if "lib" is added as crate type
 # cf. https://github.com/rust-lang/rust/issues/51009
-crate-type = ["staticlib", "cdylib"]
+crate-type = ["lib", "staticlib", "cdylib"]


Why do you need lib? Note that due to the above comment, you will lose LTO and your library size will increase.

This is a just a temporary hack to hijack the current build script and re-esxport ddtelemetry-ffi inside the profiler-ffi crate, to build and the telemetry example using the artifact built by the build-profiler-ffi script

Once I figure out how to actually build the library, I'll remove this

ivoanjo

Hey Paul 👋! Since you had asked feedback on the API (sorry it took a week), I looked at this file and the generated telemetry.h from the POV of "I'm given this thing to integrate with on my library".

So my comments are all from that POV. In particular, when I had a question, if it wasn't on this file, or on the header, I purposely did not go one level deeper into the Rust code, to hopefully give you the POV of a (*cof* very *cof*) lazy person that needs to get the job done and what was going through my head.

API design in C is for sure awful lol.

Also, I saw that in an earlier version you were using C, and now moved to C++ (although you still list the version as "C89"). I would actually suggest going back to C, as I'd expect most libraries will either be Rust or C, so I think you'd get a more realistic example.

(I am aware the profiler example is C++ ahaha. But I wish it was C as well).

ivoanjo · 2022-09-30T12:44:53Z

examples/ffi/telemetry.cpp

+  ddog_TelemetryWorkerBuilder* builder = NULL;
+
+  ddog_builder_instantiate(
+    &builder,
+    DDOG_CHARSLICE_C("ffi-test"),
+    DDOG_CHARSLICE_C("cpp"),
+    DDOG_CHARSLICE_C("C89"),
+    DDOG_CHARSLICE_C("0.8")
+  );


From a consumer POV this API seems a bit awkward -- could ddog_builder_instantiate return the builder directly, rather than having the weird pointer passed in?

ivoanjo · 2022-09-30T12:46:48Z

examples/ffi/telemetry.cpp

+  };
+  ddog_TelemetryWorkerBuilder* builder = NULL;
+
+  ddog_builder_instantiate(


The ddog_builder prefix here seems a bit weird -- should it be ddog_TelemetryBuildder or ddog_TelemetryWorkerBuilder instead?

Also, a bit more minor, but profiling has usually been using _new instead of the more verbose _instantiate.

ivoanjo · 2022-09-30T12:51:05Z

examples/ffi/telemetry.cpp

+  // Set properties by name
+  ddog_builder_with_runtime_id(builder, DDOG_CHARSLICE_C("58a260d7-4309-45bc-a167-7a64abea3da6"));
+  // Set property from enum value
+  ddog_builder_with_property(builder, DDOG_TELEMETRY_WORKER_BUILDER_PROPERTY_APPLICATION_SERVICE_VERSION,DDOG_CHARSLICE_C("0.0.1"));


I was a bit confused by this, but then realized the intention here is to show that there are two styles. I'll suggest calling it out more clearly in the comments, perhaps even setting the same property twice in the two ways to make it clear how it goes.

ivoanjo · 2022-09-30T12:52:36Z

examples/ffi/telemetry.cpp

+  TRY(
+    ddog_builder_with_str_property(builder, DDOG_CHARSLICE_C("application.env"), DDOG_CHARSLICE_C("test")),
+    "Setting key application.env on builder"
+  );


Could you clarify what's the semantic meaning of failing to set an str property? E.g. if I was writing an actual production integration with the API, what should I do with a failure here?

Same for other APIs -- I'm looking at the generated .h file and it's not clear to me at all how should I proceed when I get a ddog_MaybeError.

ivoanjo · 2022-09-30T12:55:43Z

examples/ffi/telemetry.cpp

+  TRY(
+    ddog_handle_start(handle),
+    "Starting the worker"
+  );


What's the difference between run and start again? And when in the application life cycle should they be called? And what happens if my application forks?

ivoanjo · 2022-09-30T12:58:24Z

examples/ffi/telemetry.cpp

+    DDOG_CHARSLICE_C("libdatadog.test"),
+    ddog_Vec_tag_new(),
+    DDOG_METRIC_TYPE_COUNT,
+    false,


What is this common argument, and what does it represent?

ivoanjo · 2022-09-30T13:03:42Z

examples/ffi/telemetry.cpp

+  ddog_handle_add_point(handle, &libbdatadog_test_key, 1.0, ddog_Vec_tag_new());
+  ddog_handle_add_point(handle, &libbdatadog_test_key, 2.0, ddog_Vec_tag_new());


Since ddog_ContextKey seems to be a struct with just an integer, consider passing it here by value (e.g. add_point(handle, test_key, ...)) rather than by reference :)

Also, is it me, or a) Is the code screaming for a "empty tags" constant (or just null) and b) Are you (potentially?) leaking the empty tags vector?

ivoanjo · 2022-09-30T13:06:27Z

examples/ffi/telemetry.cpp

+  ddog_handle_wait_for_shutdown(handle);
+  return 0;


Does the handle become invalid after this? Or is there any cleanup needed?

Also, similar question for the ddog_TelemetryWorkerBuilder -- did we miss cleaning it up, or was it cleaned up automatically when ddog_builder_run was called?

(And I suggest answering these as comments in the .h 😛)

ivoanjo · 2022-09-30T13:07:19Z

examples/ffi/telemetry.cpp

+  if (parsed_tags.error_message != NULL) {
+    print_error("Parsing tags", parsed_tags.error_message);
+  }
+  ddog_handle_add_point(handle, &libbdatadog_test_key, 7.0, parsed_tags.tags);


Like other functions, I have no idea what to do if this fails. Should I retry? Just shut down everything? Just ignore it?

ivoanjo · 2022-09-30T13:11:39Z

examples/ffi/telemetry.cpp

@@ -0,0 +1,96 @@
+// Unless explicitly stated otherwise all files in this repository are licensed


Hey Paul 👋! Since you had asked feedback on the API (sorry it took a week), I looked at this file and the generated telemetry.h from the POV of "I'm given this thing to integrate with on my library".

So my comments are all from that POV. In particular, when I had a question, if it wasn't on this file, or on the header, I purposely did not go one level deeper into the Rust code, to hopefully give you the POV of a (*cof* very *cof*) lazy person that needs to get the job done and what was going through my head.

API design in C is for sure awful lol.

Also, I saw that in an earlier version you were using C, and now moved to C++ (although you still list the version as "C89"). I would actually suggest going back to C, as I'd expect most libraries will either be Rust or C, so I think you'd get a more realistic example.

(I am aware the profiler example is C++ ahaha. But I wish it was C as well).

ivoanjo · 2022-09-30T13:14:27Z

Also, have you considered doing an actual implementation for one of the languages that are using libdatadog? I think having an actual, real API consumer would help uncover some of the sharp edges and issues earlier.

paullegranddc added 7 commits September 23, 2022 18:28

Remove unused settings

f25b947

Remove unnecessary error return from ffi functions

fcf87be

Don't send empty payload on worker stop

c5d1531

Implement metrics ffi

8b7aadb

WIP hack the build script to build telemetry in artifact and run example

0e1a437

lint

8f27768

pass example to cpp

e67d17c

morrisonlevi reviewed Sep 26, 2022

View reviewed changes

ivoanjo reviewed Sep 30, 2022

View reviewed changes

paullegranddc closed this Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paullgdc/telemetry ffi again #54

Paullgdc/telemetry ffi again #54

paullegranddc commented Sep 23, 2022

morrisonlevi Sep 26, 2022

paullegranddc Sep 26, 2022

ivoanjo left a comment

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo Sep 30, 2022

ivoanjo commented Sep 30, 2022

		ddog_handle_add_point(handle, &libbdatadog_test_key, 1.0, ddog_Vec_tag_new());
		ddog_handle_add_point(handle, &libbdatadog_test_key, 2.0, ddog_Vec_tag_new());

		@@ -0,0 +1,96 @@
		// Unless explicitly stated otherwise all files in this repository are licensed

Paullgdc/telemetry ffi again #54

Paullgdc/telemetry ffi again #54

Conversation

paullegranddc commented Sep 23, 2022

What does this PR do?

Motivation

Additional Notes

How to test the change?

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

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

ivoanjo commented Sep 30, 2022