Integrate nanopb to encode and decode metrics by anchitj · Pull Request #4388 · confluentinc/librdkafka

Anchit Jain (anchitj) · 2023-08-08T07:45:17Z

Changes done:

Added nanopb files in src/nanopb from https://github.com/nanopb/nanopb
Generated protobuf stubs using protoc compiler in nanopb and opentelemetry:1.0.0 protos, and added them in src/opentelemetry. One of the file metrics.proto needed an option present in metrics.options to create the callback needed for oneof types in .proto file which isn't generated by default.
Wrote encode related methods in rdkafka_telemetry_encode.c. Sample payload for a sum type metric will be like

metrics_data {
  resource_metrics {
    resource {
      attributes {
        key: "service.name"
        value {
          string_value: "example-service"
        }
      }
    }
    scope_metrics {
      scope {
        name: "example-scope"
        version: "1.0.0"
      }
      metrics {
        name: "requests_total"
        description: "Total number of requests"
        unit: "1"
        data {
          sum {
            data_points {
              attributes {
                key: "endpoint"
                value {
                  string_value: "/home"
                }
              }
              time_unix_nano: 1609462800123456789 
              value {
                as_int: 150 
              }
            }
            aggregation_temporality: AGGREGATION_TEMPORALITY_CUMULATIVE
          }
        }
      }
    }
  }
}

Wrote decode related methods in rdkafka_telemetry_decode.c. This is needed for testing, though not needed for functionality.
Only one metric is being encode right now by default.

TODO:

Add prefix to both nanopb and opentelemetry structs to not cause conflicts with existing applications dependent on them.

cla-assistant · 2023-08-08T07:45:30Z

All committers have signed the CLA.

Milind L (milindl)

Some comments, but will discuss this more with anchit

Anchit Jain (anchitj) · 2023-08-10T07:11:55Z

@@ -0,0 +1 @@
+opentelemetry.proto.metrics.v1.Metric submsg_callback:true;


Add comment here

Milind L (milindl)

Looks good, but left some comments.
Comments contain mostly minor stuff, but a few larger changes.

Milind L (milindl) · 2023-08-10T08:36:39Z


        struct {
-                /* TODO: fill this out. */
+                rd_atomic32_t connects; /**< Connection attempts,


not urgent but for later, if this is only ever calculated in the main thread, we can make it int32_t (for the historic rkc_c) to prevent any overhead of using atomic values.

Makes sense, update to int32_t.

Milind L (milindl) · 2023-08-10T08:38:50Z

                     sizeof(rd_bool_t) + strlen(compression_type) +
                     metrics_size;
-        rkbuf = rd_kafka_buf_new_request(rkb, RD_KAFKAP_PushTelemetry, 1, len);
+        rkbuf = rd_kafka_buf_new_flexver_request(rkb, RD_KAFKAP_PushTelemetry,


Not this line exactly, but above, in rd_kafka_broker_ApiVersion_supported( rkb, RD_KAFKAP_PushTelemetry, 0, 1, NULL);, we should use rd_kafka_broker_ApiVersion_supported( rkb, RD_KAFKAP_PushTelemetry, 0, 0, NULL); (set max ver = 0, not 1).

Thanks, updated.

Milind L (milindl) · 2023-08-10T08:40:11Z

+        rd_kafka_buf_write_kbytes(rkbuf, metric_bytes);
+        rd_free(metric_bytes);

        /* Let the handler perform retries so that it can pick


Comment isn't relevant, should be changed, though I believe NO_RETRIES is legitimate.

Removed the comment.

Milind L (milindl) · 2023-08-10T08:42:06Z

-        void *metrics_payload  = NULL;
+        size_t metrics_payload_size;
+        void *metrics_payload  = encode_metrics(rk, &metrics_payload_size);
        char *compression_type = "gzip";


we should ideally use rd_kafka_compression_t for PushTelemetry request. We can defer this change to later, when we actually implement compression.

Right. I've added a TODO for this.

Milind L (milindl) · 2023-08-10T08:42:13Z

        // TODO: Update dummy values
-        void *metrics_payload  = NULL;
+        size_t metrics_payload_size;
+        void *metrics_payload  = encode_metrics(rk, &metrics_payload_size);


Should be const void*

Thanks, updated.

Milind L (milindl) · 2023-08-10T08:44:29Z

+#include "nanopb/pb_decode.h"
+#include "opentelemetry/metrics.pb.h"
+
+rd_bool_t


All methods which aren't used by other files, mark them as static, here and elsewhere
That way they won't interfere with functions of the same name elsewhere (encode_string might be a fairly common name for example)

Makes sense, missed doing this. Made all the file level functions static.

Milind L (milindl) · 2023-08-10T08:45:55Z

+                                   void *const *arg) {
+        opentelemetry_proto_metrics_v1_NumberDataPoint *data_point =
+            (opentelemetry_proto_metrics_v1_NumberDataPoint *)*arg;
+        if (!pb_encode_tag_for_field(stream, field)) {


here and elsewhere, convention is to avoid using braces for single line if/for/whiles

Updated to this convention everywhere.

Milind L (milindl) · 2023-08-10T08:47:57Z

+            stream, opentelemetry_proto_common_v1_KeyValue_fields, key_value);
+}
+
+static const char *rd_kafka_type2str(rd_kafka_type_t type) {


best to move this to rdkafka_int.h and rdkafka_int.c and mark as non-static (I see it's used in rdkafka.c also)

Moved to rdkafka_int.h

Milind L (milindl) · 2023-08-10T08:54:11Z

+}
+
+// TODO: Update to handle multiple data points.
+rd_bool_t encode_number_data_point(pb_ostream_t *stream,


I am sorry about my earlier comment about making them rd_bool_t. I didn't realize they were being used as callbacks for something that expects function ptrs with return type bool(_Bool). It's best to match the callback type in that case. Otherwise the compiler will complain (warning)

Updated to bool.

Milind L (milindl) · 2023-08-10T08:57:05Z

+             *metric_description =
+                 "The total number of connections established.",
+             *metric_unit     = "1",
+             *metric_type_str = rd_kafka_type2str(rk->rk_type), *metric_name;


I think for all these things, the type should be const char* and not char*, except for metric_name which should be declared separately.

Thanks, updated.

Anchit Jain (anchitj) added 5 commits August 8, 2023 12:56

Serialise metrics using nanopb

49a9a1f

Move nanopb and opentelemetry inside src

3f00dfd

Add metrics.options file

d07605e

Remove unused includes

fd72703

Style fix

083e416

Anchit Jain (anchitj) requested a review from Milind L (milindl) August 8, 2023 07:47

Anchit Jain (anchitj) force-pushed the dev_kip_714_serialisation branch from f02522c to 3058de6 Compare August 8, 2023 08:01

Fix formatting

f14bced

Anchit Jain (anchitj) force-pushed the dev_kip_714_serialisation branch from 3058de6 to f14bced Compare August 8, 2023 08:02

Skip copyright check

8054c4a

Milind L (milindl) reviewed Aug 8, 2023

View reviewed changes

Comment thread src/rdkafka_mock_handlers.c Outdated

Comment thread src/rdkafka_telemetry.c Outdated

Comment thread src/rdkafka_telemetry_encode.c Outdated

Comment thread src/rdkafka_telemetry_encode.c Outdated

Comment thread src/Makefile

Comment thread src/rdkafka_telemetry_decode.c Outdated

Anchit Jain (anchitj) force-pushed the dev_kip_714_serialisation branch 2 times, most recently from c021826 to f288c9b Compare August 8, 2023 10:24

Add nanopb and opentelemetry in windows vcxproj

0bcf681

Anchit Jain (anchitj) force-pushed the dev_kip_714_serialisation branch from f288c9b to 0bcf681 Compare August 8, 2023 10:56

Anchit Jain (anchitj) added 7 commits August 8, 2023 16:47

Include headers directories in CMAKE

ac7b22b

Use flexver with PushTelemetry

dcf6027

Fix memory leaks

54a08a2

Change import path

cff475a

Use rd_bool_t everywhere

73363c3

Fix librdkafka.vcxproj

427b247

Use rd_bool_t

6e9e218

Anchit Jain (anchitj) commented Aug 10, 2023

View reviewed changes

Milind L (milindl) approved these changes Aug 10, 2023

View reviewed changes

PR Feedback

d83f3b5

Anchit Jain (anchitj) force-pushed the dev_kip_714_serialisation branch from 61d44ad to d83f3b5 Compare August 10, 2023 09:41

Add nanopb license

092b429

Anchit Jain (anchitj) force-pushed the dev_kip_714_serialisation branch from 0eb2dae to 092b429 Compare August 10, 2023 09:57

Include opentelemtry license

b15f017

Milind L (milindl) approved these changes Aug 21, 2023

View reviewed changes

Anchit Jain (anchitj) merged commit 530eb0b into dev_kip_714 Aug 21, 2023

Anchit Jain (anchitj) deleted the dev_kip_714_serialisation branch August 21, 2023 07:16

		@@ -0,0 +1 @@
		opentelemetry.proto.metrics.v1.Metric submsg_callback:true; No newline at end of file

Conversation

Anchit Jain (anchitj) commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes done:

TODO:

Uh oh!

cla-assistant Bot commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Milind L (milindl) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Milind L (milindl) left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Anchit Jain (anchitj) commented Aug 8, 2023 •

edited

Loading

cla-assistant Bot commented Aug 8, 2023 •

edited

Loading

Milind L (milindl) left a comment •

edited

Loading