Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle delta temporality and some code refactoring #4410

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Handle delta temporality and some code refactoring #4410

merged 6 commits into from
Aug 29, 2023

Conversation

anchitj
Copy link
Member

@anchitj anchitj commented Aug 25, 2023

No description provided.

@anchitj anchitj requested a review from milindl August 25, 2023 11:19
@anchitj anchitj marked this pull request as ready for review August 25, 2023 11:19
milindl
milindl reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, but no big changes

src/rdkafka_int.h Outdated
@@ -267,6 +267,13 @@ static RD_UNUSED const char *rd_kafka_type2str(rd_kafka_type_t type) {
return types[type];
}

typedef enum {
METRIC_CONNECTION_CREATION_TOTAL,
METRIC_CONNECTION_CREATION_RATE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix enum names with RD_KAFKA_TELEMETRY_

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

src/rdkafka_int.h Outdated
METRIC_CONNECTION_CREATION_TOTAL,
METRIC_CONNECTION_CREATION_RATE,
// add more metrics here
METRIC_COUNT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in most places, the last element is named something like __CNT, would make sense to do that here too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

src/rdkafka_telemetry_encode.h Outdated
@@ -29,6 +29,11 @@
#ifndef _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H
#define _RDKAFKA_RDKAFKA_TELEMETRY_ENCODE_H

typedef enum {
METRIC_TYPE_SUM,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as earlier, prefix with RD_KAFKA_TELEMETRY

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

src/rdkafka_telemetry.c Outdated
@@ -112,6 +115,27 @@ static void rd_kafka_telemetry_set_terminated(rd_kafka_t *rk) {
mtx_unlock(&rk->rk_telemetry.lock);
}

static rd_kafka_telemetry_metric_name_t *
rd_kafka_match_requested_metrics(rd_kafka_t *rk) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Updated to return void and modify matched_metrics inside the function itself.

src/rdkafka_telemetry.c Outdated
@@ -209,8 +232,7 @@ static void rd_kafka_send_push_telemetry(rd_kafka_t *rk,
metrics_payload, metrics_payload_size,
NULL, 0, RD_KAFKA_REPLYQ(rk->rk_ops, 0),
rd_kafka_handle_PushTelemetry, NULL);
rd_free(metrics_payload);

rd_free((void *)metrics_payload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove const from const void *metrics_payload instead of making this cast here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

src/rdkafka_telemetry_encode.c Outdated
switch (METRICS[metrics_to_encode[i]].type) {

case METRIC_TYPE_SUM: {
sum = rd_malloc(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use rd_calloc rather than memset

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

src/rdkafka_telemetry_encode.c Outdated
break;
}
case METRIC_TYPE_GAUGE: {
gauge = rd_malloc(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use rd_calloc rather than memset

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

src/rdkafka_telemetry_encode.c Outdated
sum->is_monotonic = true;
metrics[i]->which_data =
opentelemetry_proto_metrics_v1_Metric_sum_tag;
metrics[i]->data.sum = *sum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Updated for both

src/rdkafka_telemetry_encode.c
break;
}
default:
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rd_assert(!*"<some error message saying this should be impossible>");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/rdkafka_telemetry_encode.c Outdated
metric_name_len = strlen(metric_type_str) +
strlen(METRICS[metrics_to_encode[i]].name) +
2;
metric_names[i] = rd_malloc(metric_name_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rd_calloc rather than malloc and memset

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@anchitj anchitj force-pushed the dev_kip_714_delta branch 2 times, most recently from 625b4e2 to 11874b2 Compare August 28, 2023 12:23
@anchitj anchitj merged commit 5c943ba into dev_kip_714 Aug 29, 2023
1 check passed
@anchitj anchitj deleted the dev_kip_714_delta branch August 29, 2023 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milindl milindl milindl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anchitj @milindl