From 1f9301de5b2816c7e2d0413ea2ce627f3375eec5 Mon Sep 17 00:00:00 2001 From: Greg Eales <0x006EA1E5@gmail.com> Date: Mon, 4 Nov 2024 17:34:42 +0000 Subject: [PATCH] Code Review Signed-off-by: Greg Eales <0x006EA1E5@gmail.com> --- plugins/out_loki/loki.c | 58 +++++++++++++++++++++------------- plugins/out_loki/loki.h | 14 ++++----- tests/runtime/out_loki.c | 68 ++++++++++++++++++++++++++-------------- 3 files changed, 88 insertions(+), 52 deletions(-) diff --git a/plugins/out_loki/loki.c b/plugins/out_loki/loki.c index 644c31f1158..1043f6f93be 100644 --- a/plugins/out_loki/loki.c +++ b/plugins/out_loki/loki.c @@ -423,10 +423,11 @@ static void pack_kv(struct flb_loki *ctx, } } -/* Similar to pack_kv above, except will only use msgpack_objects of type - * MSGPACK_OBJECT_MAP, and will iterate over the keys adding each entry as a separate - * item. Non-string map values are serialised to JSON, as Loki requires all values to be - * strings. +/* + * Similar to pack_kv above, except will only use msgpack_objects of type + * MSGPACK_OBJECT_MAP, and will iterate over the keys adding each entry as a + * separate item. Non-string map values are serialised to JSON, as Loki requires + * all values to be strings. */ static void pack_maps(struct flb_loki *ctx, msgpack_packer *mp_pck, @@ -460,17 +461,20 @@ static void pack_maps(struct flb_loki *ctx, if (flb_ra_get_kv_pair(kv->ra_key, *map, &start_key, &out_key, &out_val) == MSGPACK_UNPACK_CONTINUE) { - /* we require the value to be a map, or it doesn't make sense as this is - * adding a map's key / values */ + /* + * we require the value to be a map, or it doesn't make sense as + * this is adding a map's key / values + */ if (out_val->type != MSGPACK_OBJECT_MAP || out_val->via.map.size <= 0) { flb_plg_debug(ctx->ins, "No valid map data found for key %s", - kv->ra_key->pattern); - } else { + kv->ra_key->pattern); + } + else { accessed_map = out_val->via.map; /* for each entry in the accessed map... */ for (accessed_map_index = 0; accessed_map_index < accessed_map.size; - ++accessed_map_index) { + accessed_map_index++) { /* get the entry */ accessed_map_kv = accessed_map.ptr[accessed_map_index]; @@ -479,21 +483,25 @@ static void pack_maps(struct flb_loki *ctx, flb_mp_map_header_append(mh); pack_label_key(mp_pck, (char*) accessed_map_kv.key.via.str.ptr, - accessed_map_kv.key.via.str.size); - /* Does this need optimising? For example, to handle bool as - * non-JSON? */ + accessed_map_kv.key.via.str.size); + /* + * Does this need optimising? For example, to handle + * bool as non-JSON? + */ if (accessed_map_kv.val.type == MSGPACK_OBJECT_STR) { msgpack_pack_str_with_body(mp_pck, - accessed_map_kv.val.via.str.ptr, - accessed_map_kv.val.via.str.size); + accessed_map_kv.val.via.str.ptr, + accessed_map_kv.val.via.str.size); } - /* convert value to JSON, as Loki expects a string value */ + /* + * convert value to JSON, as Loki expects a string value + */ else { accessed_map_val_json = flb_msgpack_to_json_str(1024, &accessed_map_kv.val); if (accessed_map_val_json) { msgpack_pack_str_with_body(mp_pck, accessed_map_val_json, - strlen(accessed_map_val_json)); + strlen(accessed_map_val_json)); flb_free(accessed_map_val_json); } } @@ -513,9 +521,13 @@ static flb_sds_t pack_structured_metadata(struct flb_loki *ctx, /* Initialize dynamic map header */ flb_mp_map_header_init(&mh, mp_pck); if (ctx->structured_metadata_map_keys) { - pack_maps(ctx, mp_pck, tag, tag_len, map, &mh, &ctx->structured_metadata_map_keys_list); + pack_maps(ctx, mp_pck, tag, tag_len, map, &mh, + &ctx->structured_metadata_map_keys_list); } - // explicit structured_metadata entries override structured_metadata_map_keys entries + /* + * explicit structured_metadata entries override + * structured_metadata_map_keys entries + * */ if (ctx->structured_metadata) { pack_kv(ctx, mp_pck, tag, tag_len, map, &mh, &ctx->structured_metadata_list); } @@ -897,11 +909,13 @@ static int parse_labels(struct flb_loki *ctx) entry = mk_list_entry(head, struct flb_slist_entry, _head); if (entry->str[0] != '$') { flb_plg_error(ctx->ins, - "invalid structured metadata map key, the name must start with '$'"); + "invalid structured metadata map key, the name must start " + "with '$'"); return -1; } - ret = flb_loki_kv_append(ctx, &ctx->structured_metadata_map_keys_list, entry->str, NULL); + ret = flb_loki_kv_append(ctx, &ctx->structured_metadata_map_keys_list, + entry->str, NULL); if (ret == -1) { return -1; } @@ -1656,7 +1670,7 @@ static flb_sds_t loki_compose_payload(struct flb_loki *ctx, &log_decoder, &log_event)) == FLB_EVENT_DECODER_SUCCESS) { msgpack_pack_array(&mp_pck, ctx->structured_metadata || - ctx->structured_metadata_map_keys ? 3 : 2); + ctx->structured_metadata_map_keys ? 3 : 2); /* Append the timestamp */ pack_timestamp(&mp_pck, &log_event.timestamp); @@ -1693,7 +1707,7 @@ static flb_sds_t loki_compose_payload(struct flb_loki *ctx, msgpack_pack_array(&mp_pck, 1); msgpack_pack_array(&mp_pck, ctx->structured_metadata || - ctx->structured_metadata_map_keys ? 3 : 2); + ctx->structured_metadata_map_keys ? 3 : 2); /* Append the timestamp */ pack_timestamp(&mp_pck, &log_event.timestamp); diff --git a/plugins/out_loki/loki.h b/plugins/out_loki/loki.h index dd434fcc231..69b539f9991 100644 --- a/plugins/out_loki/loki.h +++ b/plugins/out_loki/loki.h @@ -88,14 +88,14 @@ struct flb_loki { char *tcp_host; int out_line_format; int out_drop_single_key; - int ra_used; /* number of record accessor label keys */ - struct flb_record_accessor *ra_k8s; /* kubernetes record accessor */ - struct mk_list labels_list; /* list of flb_loki_kv nodes */ - struct mk_list structured_metadata_list; /* list of flb_loki_kv nodes */ + int ra_used; /* number of record accessor label keys */ + struct flb_record_accessor *ra_k8s; /* kubernetes record accessor */ + struct mk_list labels_list; /* list of flb_loki_kv nodes */ + struct mk_list structured_metadata_list; /* list of flb_loki_kv nodes */ struct mk_list structured_metadata_map_keys_list; /* list of flb_loki_kv nodes */ - struct mk_list remove_keys_derived; /* remove_keys with label RAs */ - struct flb_mp_accessor *remove_mpa; /* remove_keys multi-pattern accessor */ - struct flb_record_accessor *ra_tenant_id_key; /* dynamic tenant id key */ + struct mk_list remove_keys_derived; /* remove_keys with label RAs */ + struct flb_mp_accessor *remove_mpa; /* remove_keys multi-pattern accessor */ + struct flb_record_accessor *ra_tenant_id_key; /* dynamic tenant id key */ struct cfl_list dynamic_tenant_list; pthread_mutex_t dynamic_tenant_list_lock; diff --git a/tests/runtime/out_loki.c b/tests/runtime/out_loki.c index 014fe891cd8..a15e1b43965 100644 --- a/tests/runtime/out_loki.c +++ b/tests/runtime/out_loki.c @@ -739,6 +739,7 @@ static void cb_check_float_value(void *ctx, int ffd, flb_sds_destroy(out_js); } + #define JSON_FLOAT "[12345678, {\"float\":1.3}]" void flb_test_float_value() { @@ -804,10 +805,14 @@ static void cb_check_structured_metadata_value(void *ctx, int ffd, flb_sds_destroy(out_js); } -#define JSON_MAP "[12345678, {\"log\": \"This is an interesting log message!\", \"map1\": {\"key1\": \"value1\", \"key2\": \"value2\", \"key_nested_object_1\": {\"sub_key1\": \"sub_value1\", \"sub_key2\": false}}, \"map2\": {\"key4\": \"value1\", \"key5\": false}, \"map3\": {\"key1\": \"map3_value1\", \"key2\": \"map3_value2\"}}]" -void flb_test_structured_metadata_map_params(char *log_level, char *remove_keys, - char *structured_metadata, char *structured_metadata_map_keys, char *input_log_json, - char *expected_sub_str) +#define JSON_MAP "[12345678, {\"log\": \"This is an interesting log message!\", " \ + "\"map1\": {\"key1\": \"value1\", \"key2\": \"value2\", \"key_nested_object_1\": " \ + "{\"sub_key1\": \"sub_value1\", \"sub_key2\": false}}, \"map2\": {\"key4\": " \ + "\"value1\", \"key5\": false}, \"map3\": {\"key1\": \"map3_value1\", \"key2\": " \ + "\"map3_value2\"}}]" +void flb_test_structured_metadata_map_params(char *remove_keys, + char *structured_metadata, char *structured_metadata_map_keys, char *input_log_json, + char *expected_sub_str) { int ret; size_t size = strlen(input_log_json); @@ -818,7 +823,7 @@ void flb_test_structured_metadata_map_params(char *log_level, char *remove_keys, /* Create context, flush every second (some checks omitted here) */ ctx = flb_create(); flb_service_set(ctx, "flush", "1", "grace", "1", - "log_level", log_level ? log_level : "error", + "log_level", "error", NULL); /* Lib input mode */ @@ -830,11 +835,11 @@ void flb_test_structured_metadata_map_params(char *log_level, char *remove_keys, flb_output_set(ctx, out_ffd, "match", "test", "line_format", "key_value", - "remove_keys", remove_keys ? remove_keys : "", + "remove_keys", remove_keys, "drop_single_key", "on", "labels", "service_name=my_service_name", - "structured_metadata", structured_metadata ? structured_metadata : "", - "structured_metadata_map_keys", structured_metadata_map_keys ? structured_metadata_map_keys : "", + "structured_metadata", structured_metadata, + "structured_metadata_map_keys", structured_metadata_map_keys, NULL); /* Enable test mode */ @@ -855,61 +860,75 @@ void flb_test_structured_metadata_map_params(char *log_level, char *remove_keys, flb_destroy(ctx); } -#define LOG_LEVEL "error" void flb_test_structured_metadata_map_single_map() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "", "$map1", JSON_MAP, - "{\"key1\":\"value1\",\"key2\":\"value2\",\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\",\\\"sub_key2\\\":false}\"}"); + "{\"key1\":\"value1\",\"key2\":\"value2\"," + "\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\"," + "\\\"sub_key2\\\":false}\"}"); } void flb_test_structured_metadata_map_two_maps() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "", "$map1,$map2", JSON_MAP, - "{\"key1\":\"value1\",\"key2\":\"value2\",\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\",\\\"sub_key2\\\":false}\",\"key4\":\"value1\",\"key5\":\"false\"}"); + "{\"key1\":\"value1\",\"key2\":\"value2\"," + "\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\"," + "\\\"sub_key2\\\":false}\",\"key4\":\"value1\",\"key5\":\"false\"}"); +} + +void flb_test_structured_metadata_map_sub_map() { + flb_test_structured_metadata_map_params( + "map1, map2, map3", + "", + "$map1['key_nested_object_1']", + JSON_MAP, + "\"This is an interesting log message!\",{\"sub_key1\":\"sub_value1\"," + "\"sub_key2\":\"false\"}"); } void flb_test_structured_metadata_map_both_with_non_map_value() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "$map2", "$map1,$map2", JSON_MAP, - "{\"key1\":\"value1\",\"key2\":\"value2\",\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\",\\\"sub_key2\\\":false}\",\"key4\":\"value1\",\"key5\":\"false\",\"map2\":\"{\\\"key4\\\":\\\"value1\\\",\\\"key5\\\":false}\"}"); + "{\"key1\":\"value1\",\"key2\":\"value2\"," + "\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\"," + "\\\"sub_key2\\\":false}\",\"key4\":\"value1\",\"key5\":\"false\"," + "\"map2\":\"{\\\"key4\\\":\\\"value1\\\",\\\"key5\\\":false}\"}"); } /* key1 is overridden by the explicit value given to structured_metadata */ void flb_test_structured_metadata_map_value_explicit_override_map_key() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "key1=value_explicit", "$map1,$map2", JSON_MAP, - "{\"key2\":\"value2\",\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\",\\\"sub_key2\\\":false}\",\"key4\":\"value1\",\"key5\":\"false\",\"key1\":\"value_explicit\"}"); + "{\"key2\":\"value2\"," + "\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\"," + "\\\"sub_key2\\\":false}\",\"key4\":\"value1\",\"key5\":\"false\"," + "\"key1\":\"value_explicit\"}"); } void flb_test_structured_metadata_explicit_only_no_map() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "key1=value_explicit", "", JSON_MAP, - "[\"12345678000000000\",\"This is an interesting log message!\",{\"key1\":\"value_explicit\"}]"); + "[\"12345678000000000\"," + "\"This is an interesting log message!\",{\"key1\":\"value_explicit\"}]"); } void flb_test_structured_metadata_explicit_only_map() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "$map2", "", @@ -919,17 +938,18 @@ void flb_test_structured_metadata_explicit_only_map() { void flb_test_structured_metadata_map_and_explicit() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "key_explicit=value_explicit", "$map1", JSON_MAP, - "[\"12345678000000000\",\"This is an interesting log message!\",{\"key1\":\"value1\",\"key2\":\"value2\",\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\",\\\"sub_key2\\\":false}\",\"key_explicit\":\"value_explicit\"}]"); + "[\"12345678000000000\",\"This is an interesting log message!\"," + "{\"key1\":\"value1\",\"key2\":\"value2\"," + "\"key_nested_object_1\":\"{\\\"sub_key1\\\":\\\"sub_value1\\\"," + "\\\"sub_key2\\\":false}\",\"key_explicit\":\"value_explicit\"}]"); } void flb_test_structured_metadata_map_single_missing_map() { flb_test_structured_metadata_map_params( - LOG_LEVEL, "map1, map2, map3", "", "$missing_map", @@ -955,6 +975,8 @@ TEST_LIST = { flb_test_structured_metadata_map_single_map}, {"structured_metadata_map_two_maps", flb_test_structured_metadata_map_two_maps}, + {"structured_metadata_map_sub_map", + flb_test_structured_metadata_map_sub_map}, {"structured_metadata_map_both_with_non_map_value", flb_test_structured_metadata_map_both_with_non_map_value}, {"structured_metadata_map_value_explicit_override_map_key",