Skip to content

mp: sort flb_record_accessor by a number of subkeys(#5546) #5590

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

Merged
edsiper merged 6 commits into from
Feb 14, 2023
Merged

mp: sort flb_record_accessor by a number of subkeys(#5546) #5590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
53817cd
tests: internal: mp: add test case for unsorted keys(#5546)
nokute78 Jun 19, 2022
a0c21b0
ra: add API to get number of subkeys
nokute78 Jun 19, 2022
8a4ded6
mp: sort flb_record_accessor by a number of subkeys
nokute78 Jun 19, 2022
3696447
mp: use mk_list_add_before
nokute78 Oct 5, 2022
c9ece83
record_accessor: replace 'num' for 'count'
nokute78 Feb 10, 2023
b98c063
mp: remove redundant initializing
nokute78 Feb 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/fluent-bit/flb_record_accessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ struct flb_record_accessor {
struct mk_list _head; /* Head to custom list (only used by flb_mp.h) */
};

int flb_ra_subkey_count(struct flb_record_accessor *ra);
struct flb_record_accessor *flb_ra_create(char *str, int translate_env);
void flb_ra_destroy(struct flb_record_accessor *ra);
void flb_ra_dump(struct flb_record_accessor *ra);
Expand Down
1 change: 1 addition & 0 deletions include/fluent-bit/record_accessor/flb_ra_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ struct flb_ra_key *flb_ra_parser_key_add(struct flb_ra_parser *ra, char *key);
int flb_ra_parser_subentry_add_string(struct flb_ra_parser *rp, char *key);
int flb_ra_parser_subentry_add_array_id(struct flb_ra_parser *rp, int id);

int flb_ra_parser_subkey_count(struct flb_ra_parser *rp);
void flb_ra_parser_dump(struct flb_ra_parser *rp);
struct flb_ra_parser *flb_ra_parser_string_create(char *str, int len);
struct flb_ra_parser *flb_ra_parser_regex_id_create(int id);
Expand Down
33 changes: 32 additions & 1 deletion src/flb_mp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,37 @@ void flb_mp_array_header_end(struct flb_mp_map_header *mh)
flb_mp_set_array_header_size(ptr, mh->entries);
}

static int insert_by_subkey_count(struct flb_record_accessor *ra, struct flb_mp_accessor *mpa)
{
int subkey_count;
int count;
struct mk_list *h;
struct flb_record_accessor *val_ra;

/*
* sort flb_record_accessor by number of subkey
*
* e.g.
* $kubernetes
* $kubernetes[2]['a']
* $kubernetes[2]['annotations']['fluentbit.io/tag']
*/
subkey_count = flb_ra_subkey_count(ra);
mk_list_foreach(h, &mpa->ra_list) {
val_ra = mk_list_entry(h, struct flb_record_accessor, _head);
count = flb_ra_subkey_count(val_ra);
if (count >= subkey_count) {
mk_list_add_before(&ra->_head, &val_ra->_head, &mpa->ra_list);
return 0;
}
}

/* add to tail of list */
mk_list_add(&ra->_head, &mpa->ra_list);
return 0;
}


/*
* Create an 'mp accessor' context: this context allows to create a list of
* record accessor patterns based on a 'slist' context, where every slist string
Expand Down Expand Up @@ -382,7 +413,7 @@ struct flb_mp_accessor *flb_mp_accessor_create(struct mk_list *slist_patterns)
flb_mp_accessor_destroy(mpa);
return NULL;
}
mk_list_add(&ra->_head, &mpa->ra_list);
insert_by_subkey_count(ra, mpa);
}

if (mk_list_size(&mpa->ra_list) == 0) {
Expand Down
21 changes: 21 additions & 0 deletions src/flb_record_accessor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,27 @@ void flb_ra_destroy(struct flb_record_accessor *ra)
flb_free(ra);
}

int flb_ra_subkey_count(struct flb_record_accessor *ra)
{
struct mk_list *head;
struct flb_ra_parser *rp;
int ret = -1;
int tmp;

if (ra == NULL) {
Copy link
Contributor

@leonardo-albertovich leonardo-albertovich Feb 7, 2023

Choose a reason for hiding this comment

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

Maybe returning 0 in this case would be better even if that would make it undistinguishable from legitimate empty ra it would be it less error prone (ie. someone using a size_t without paying attention) since at the moment the result is not being checked.

Copy link
Contributor Author

@nokute78 nokute78 Feb 10, 2023

Choose a reason for hiding this comment

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

I prefer returning negative value.

In a future, the result can be checked.

return -1;
}
mk_list_foreach(head, &ra->list) {
rp = mk_list_entry(head, struct flb_ra_parser, _head);
tmp = flb_ra_parser_subkey_count(rp);
if (tmp > ret) {
ret = tmp;
}
}

return ret;
}

struct flb_record_accessor *flb_ra_create(char *str, int translate_env)
{
int ret;
Expand Down
15 changes: 15 additions & 0 deletions src/record_accessor/flb_ra_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,21 @@
#include "ra_parser.h"
#include "ra_lex.h"

int flb_ra_parser_subkey_count(struct flb_ra_parser *rp)
{
if (rp == NULL || rp->key == NULL) {
return -1;
}
else if (rp->type != FLB_RA_PARSER_KEYMAP) {
return 0;
}
else if(rp->key->subkeys == NULL) {
return -1;
}

return mk_list_size(rp->key->subkeys);
Copy link
Contributor

@leonardo-albertovich leonardo-albertovich Sep 14, 2022

Choose a reason for hiding this comment

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

Beware of mk_list_size, it iterates the entire list in order to calculate the size each time. At this point I think we might just waste 8 bytes per node and add a counter that's only used in the head node but I haven't properly assessed the impact of it yet.

Copy link
Contributor Author

@nokute78 nokute78 Sep 17, 2022

Choose a reason for hiding this comment

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

I'm glad monkey API supports that feature.

However we don't pass a head node pointer to mk_list_del function.
I think we need to seek the head node from a entry pointer or create a new API to pass a head node to decrement list size.

Copy link
Contributor

@leonardo-albertovich leonardo-albertovich Sep 18, 2022

Choose a reason for hiding this comment

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

That's true and the possible side effect could be a lot worse than the original issue, you're right.

}

void flb_ra_parser_dump(struct flb_ra_parser *rp)
{
struct mk_list *head;
Expand Down
200 changes: 200 additions & 0 deletions tests/internal/mp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,209 @@ void test_accessor_keys_remove()
msgpack_unpacked_destroy(&result);
}

/* https://github.com/fluent/fluent-bit/issues/5546 */
void test_keys_remove_subkey_key()
{
int len;
int ret;
int type;
size_t off = 0;
char *buf;
size_t size;
char *out_buf;
size_t out_size;
char *json;
char final_json[2048] = {0};
msgpack_unpacked result;
msgpack_unpacked result_final;
msgpack_object map;
struct flb_mp_accessor *mpa;
struct mk_list patterns;

/* Sample JSON message */
json =
"{\"key1\": \"something\", "
"\"kubernetes\": "
" [true, "
" false, "
" {\"a\": false, "
" \"annotations\": { "
" \"fluentbit.io/tag\": \"thetag\","
" \"extra\": false\""
"}}]}";

/* Convert to msgpack */
len = strlen(json);
ret = flb_pack_json(json, len, &buf, &size, &type);
TEST_CHECK(ret == 0);
if (ret == -1) {
exit(EXIT_FAILURE);
}

/* Unpack the content */
msgpack_unpacked_init(&result);
msgpack_unpack_next(&result, buf, size, &off);
map = result.data;

/* Create list of patterns */
flb_slist_create(&patterns);

/* sub key -> key */
flb_slist_add(&patterns, "$kubernetes[2]['annotations']['fluentbit.io/tag']");
flb_slist_add(&patterns, "$kubernetes");


/* Create mp accessor */
mpa = flb_mp_accessor_create(&patterns);
TEST_CHECK(mpa != NULL);

/* Remove the entry that matches the pattern(s) */
ret = flb_mp_accessor_keys_remove(mpa, &map, (void *) &out_buf, &out_size);
TEST_CHECK(ret == FLB_TRUE);

printf("\n=== ORIGINAL ===\n");
flb_pack_print(buf, size);
flb_free(buf);

printf("=== FINAL MAP ===\n");
if (ret == FLB_TRUE) {
flb_pack_print(out_buf, out_size);
}
msgpack_unpacked_destroy(&result);

off = 0;
msgpack_unpacked_init(&result_final);
msgpack_unpack_next(&result_final, out_buf, out_size, &off);
flb_msgpack_to_json(&final_json[0], sizeof(final_json), &result_final.data);

if (!TEST_CHECK(strstr(&final_json[0] ,"kubernetes") == NULL)) {
TEST_MSG("kubernetes field should be removed");
}

msgpack_unpacked_destroy(&result_final);

flb_free(out_buf);
flb_mp_accessor_destroy(mpa);
flb_slist_destroy(&patterns);

}

void remove_subkey_keys(char *list[], int list_size, int index_start)
{
int len;
int ret;
int type;
size_t off = 0;
char *buf;
size_t size;
char *out_buf;
size_t out_size;
char *json;
char final_json[2048] = {0};
msgpack_unpacked result;
msgpack_unpacked result_final;
msgpack_object map;
struct flb_mp_accessor *mpa;
struct mk_list patterns;
int i;
int count = 0;

/* Sample JSON message */
json =
"{\"key1\": \"something\", "
"\"kubernetes\": "
" [true, "
" false, "
" {\"a\": false, "
" \"annotations\": { "
" \"fluentbit.io/tag\": \"thetag\","
" \"extra\": false\""
"}}]}";

/* Convert to msgpack */
len = strlen(json);
ret = flb_pack_json(json, len, &buf, &size, &type);
TEST_CHECK(ret == 0);
if (ret == -1) {
exit(EXIT_FAILURE);
}

/* Unpack the content */
msgpack_unpacked_init(&result);
msgpack_unpack_next(&result, buf, size, &off);
map = result.data;

/* Create list of patterns */
flb_slist_create(&patterns);

/* sub keys */
for (i=index_start; count<list_size; i++) {
if (i>=list_size) {
i = 0;
}
flb_slist_add(&patterns, list[i]);
count++;
}

/* Create mp accessor */
mpa = flb_mp_accessor_create(&patterns);
TEST_CHECK(mpa != NULL);

/* Remove the entry that matches the pattern(s) */
ret = flb_mp_accessor_keys_remove(mpa, &map, (void *) &out_buf, &out_size);
TEST_CHECK(ret == FLB_TRUE);

printf("\n=== ORIGINAL ===\n");
flb_pack_print(buf, size);
flb_free(buf);

printf("=== FINAL MAP ===\n");
if (ret == FLB_TRUE) {
flb_pack_print(out_buf, out_size);
}
msgpack_unpacked_destroy(&result);

off = 0;
msgpack_unpacked_init(&result_final);
msgpack_unpack_next(&result_final, out_buf, out_size, &off);
flb_msgpack_to_json(&final_json[0], sizeof(final_json), &result_final.data);

if (!TEST_CHECK(strstr(&final_json[0] ,"kubernetes") == NULL)) {
TEST_MSG("kubernetes field should be removed");
}

msgpack_unpacked_destroy(&result_final);

flb_free(out_buf);
flb_mp_accessor_destroy(mpa);
flb_slist_destroy(&patterns);
}

void test_keys_remove_subkey_keys()
{
char *list[] = {"$kubernetes[2]['annotations']['fluentbit.io/tag']",
"$kubernetes[2]['a']",
"$kubernetes"};
char *list2[] = {"$kubernetes[2]['annotations']['fluentbit.io/tag']",
"$kubernetes",
"$kubernetes[2]['a']"};

int size = sizeof(list)/sizeof(char*);
int i;

for (i=0; i<size; i++) {
remove_subkey_keys(list, size, i);
}
for (i=0; i<size; i++) {
remove_subkey_keys(list2, size, i);
}
}

TEST_LIST = {
{"count" , test_count},
{"map_header" , test_map_header},
{"accessor_keys_remove" , test_accessor_keys_remove},
{"accessor_keys_remove_subkey_key" , test_keys_remove_subkey_key},
{"accessor_keys_remove_subkey_keys" , test_keys_remove_subkey_keys},
{ 0 }
};