Handle delta temporality and some code refactoring#4410
Handle delta temporality and some code refactoring#4410Anchit Jain (anchitj) merged 6 commits intodev_kip_714from
Conversation
Anchit Jain (anchitj)
force-pushed
the
dev_kip_714_delta
branch
from
7e49045 to
1e799ae
Compare
Milind L (milindl)
left a comment
Milind L (milindl)
left a comment
There was a problem hiding this comment.
Some comments, but no big changes
|
|
||
| typedef enum { | ||
| METRIC_CONNECTION_CREATION_TOTAL, | ||
| METRIC_CONNECTION_CREATION_RATE, |
There was a problem hiding this comment.
prefix enum names with RD_KAFKA_TELEMETRY_
There was a problem hiding this comment.
Updated.
| METRIC_CONNECTION_CREATION_TOTAL, | ||
| METRIC_CONNECTION_CREATION_RATE, | ||
| // add more metrics here | ||
| METRIC_COUNT |
There was a problem hiding this comment.
nit: in most places, the last element is named something like __CNT, would make sense to do that here too
There was a problem hiding this comment.
Updated.
| #define _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H | ||
|
|
||
| typedef enum { | ||
| METRIC_TYPE_SUM, |
There was a problem hiding this comment.
Same as earlier, prefix with RD_KAFKA_TELEMETRY
There was a problem hiding this comment.
Updated.
| } | ||
|
|
||
| static rd_kafka_telemetry_metric_name_t * | ||
| rd_kafka_match_requested_metrics(rd_kafka_t *rk) { |
There was a problem hiding this comment.
If we are returning rd_kafka_telemetry_metric_name_t*, we should return the size as well (by populating a size_t pointer) and take the requested metrics as a pointer (ie make this a 'pure' function), or else we should modify rk->rk_telemetry.matched_metrics inside the function itself and return void - currently we do things half-way.
There was a problem hiding this comment.
Makes sense. Updated to return void and modify matched_metrics inside the function itself.
| rd_kafka_handle_PushTelemetry, NULL); | ||
| rd_free(metrics_payload); | ||
|
|
||
| rd_free((void *)metrics_payload); |
There was a problem hiding this comment.
Remove const from const void *metrics_payload instead of making this cast here
There was a problem hiding this comment.
Updated
| switch (METRICS[metrics_to_encode[i]].type) { | ||
|
|
||
| case METRIC_TYPE_SUM: { | ||
| sum = rd_malloc( |
There was a problem hiding this comment.
Use rd_calloc rather than memset
There was a problem hiding this comment.
Updated
| break; | ||
| } | ||
| case METRIC_TYPE_GAUGE: { | ||
| gauge = rd_malloc( |
There was a problem hiding this comment.
Use rd_calloc rather than memset
There was a problem hiding this comment.
Updated
| sum->is_monotonic = true; | ||
| metrics[i]->which_data = | ||
| opentelemetry_proto_metrics_v1_Metric_sum_tag; | ||
| metrics[i]->data.sum = *sum; |
There was a problem hiding this comment.
Why do we allocate a new sum rather than changing this directly?
Like metrics[i]->data.sum.data_points.funcs.encode = ... and so on?
Same for gauge
There was a problem hiding this comment.
Good catch. Updated for both
| break; | ||
| } | ||
| default: | ||
| break; |
There was a problem hiding this comment.
rd_assert(!*"<some error message saying this should be impossible>");
| metric_name_len = strlen(metric_type_str) + | ||
| strlen(METRICS[metrics_to_encode[i]].name) + | ||
| 2; | ||
| metric_names[i] = rd_malloc(metric_name_len); |
There was a problem hiding this comment.
rd_calloc rather than malloc and memset
Anchit Jain (anchitj)
force-pushed
the
dev_kip_714_delta
branch
2 times, most recently
from
625b4e2 to
11874b2
Compare
Anchit Jain (anchitj)
force-pushed
the
dev_kip_714_delta
branch
from
11874b2 to
27dd974
Compare
2 participants
No description provided.