From a88a22166d768fbfc2da14d88fada058cfad7735 Mon Sep 17 00:00:00 2001 From: Alistair Veitch Date: Fri, 15 Jan 2016 12:13:36 -0800 Subject: [PATCH] add creation status --- include/grpc/census.h | 22 ++++--- src/core/census/tag_set.c | 72 +++++++++++++++------ test/core/census/tag_set_test.c | 107 +++++++++++++++++--------------- 3 files changed, 125 insertions(+), 76 deletions(-) diff --git a/include/grpc/census.h b/include/grpc/census.h index 1d7e65cdd9d..04b2c9db5fb 100644 --- a/include/grpc/census.h +++ b/include/grpc/census.h @@ -368,22 +368,27 @@ typedef struct { int n_modified_tags; /* number of tags that were modified */ int n_invalid_tags; /* number of tags with bad keys or values (e.g. longer than CENSUS_MAX_TAG_KV_LEN) */ -} census_tag_set_create_stats; + int n_ignored_tags; /* number of tags ignored because of + CENSUS_MAX_PROPAGATED_TAGS limit. */ +} census_tag_set_create_status; /* Create a new tag set, adding and removing tags from an existing tag set. @param base Base tag set to build upon. Can be NULL. @param tags A set of tags to be added/changed/deleted. Tags with keys that are in 'tags', but not 'base', are added to the tag set. Keys that are in - both 'tags' and 'base' will have their value replaced. Tags with keys in - both, but with NULL or zero-length values, will be deleted from the - tag set. + both 'tags' and 'base' will have their value/flags modified. Tags with keys + in both, but with NULL or zero-length values, will be deleted from the tag + set. Tags with invalid (too long or short) keys or values will be ignored. + If adding a tag will result in more than CENSUS_MAX_PROPAGATED_TAGS in either + binary or non-binary tags, they will be ignored, as will deletions of + tags that don't exist. @param ntags number of tags in 'tags' @param stats Information about the tag set created and actions taken during its creation. */ census_tag_set *census_tag_set_create(const census_tag_set *base, const census_tag *tags, int ntags, - census_tag_set_create_stats *stats); + census_tag_set_create_status *status); /* Destroy a tag set created by census_tag_set_create(). Once this function has been called, the tag set cannot be reused. */ @@ -429,9 +434,12 @@ size_t census_tag_set_encode_propagated(const census_tag_set *tags, size_t census_tag_set_encode_propagated_binary(const census_tag_set *tags, char *buffer, size_t buf_size); -/* Decode tag set buffers encoded with census_tag_set_encode_*(). */ +/* Decode tag set buffers encoded with census_tag_set_encode_*(). Returns NULL + if there is an error in parsing either buffer. The number of tags in the + decoded tag set will be returned in status, if it is non-NULL. */ census_tag_set *census_tag_set_decode(const char *buffer, size_t size, - const char *bin_buffer, size_t bin_size); + const char *bin_buffer, size_t bin_size, + census_tag_set_create_status *status); /* Get a contexts tag set. */ census_tag_set *census_context_tag_set(census_context *context); diff --git a/src/core/census/tag_set.c b/src/core/census/tag_set.c index 3b8efb19358..f5f32f5e0fb 100644 --- a/src/core/census/tag_set.c +++ b/src/core/census/tag_set.c @@ -31,9 +31,6 @@ * */ /* -- ability to add extra tags in encode? -- add drops error count to create_ts -- add mask to ntags? - comment about key/value ptrs being to mem - add comment about encode/decode being for RPC use only. */ @@ -175,9 +172,13 @@ static bool cts_delete_tag(census_tag_set *tags, const census_tag *tag, key_len)); } -// Add a tag to a tag set. -static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, +// Add a tag to a tag set. Return true on sucess, false if the tag could +// not be added because of tag size constraints. +static bool tag_set_add_tag(struct tag_set *tags, const census_tag *tag, size_t key_len) { + if (tags->ntags == CENSUS_MAX_PROPAGATED_TAGS) { + return false; + } const size_t tag_size = key_len + tag->value_len + TAG_HEADER_SIZE; if (tags->kvm_used + tag_size > tags->kvm_size) { // allocate new memory if needed @@ -199,22 +200,41 @@ static void tag_set_add_tag(struct tag_set *tags, const census_tag *tag, tags->kvm_used += tag_size; tags->ntags++; tags->ntags_alloc++; + return true; } -// Add a tag to a census_tag_set +// Add a tag to a census_tag_set. static void cts_add_tag(census_tag_set *tags, const census_tag *tag, - size_t key_len) { + size_t key_len, census_tag_set_create_status *status) { // first delete the tag if it is already present - cts_delete_tag(tags, tag, key_len); - if (tag->value != NULL && tag->value_len != 0) { + bool deleted = cts_delete_tag(tags, tag, key_len); + bool call_add = tag->value != NULL && tag->value_len != 0; + bool added = false; + if (call_add) { if (CENSUS_TAG_IS_PROPAGATED(tag->flags)) { if (CENSUS_TAG_IS_BINARY(tag->flags)) { - tag_set_add_tag(&tags->tags[PROPAGATED_BINARY_TAGS], tag, key_len); + added = + tag_set_add_tag(&tags->tags[PROPAGATED_BINARY_TAGS], tag, key_len); + } else { + added = tag_set_add_tag(&tags->tags[PROPAGATED_TAGS], tag, key_len); + } + } else { + added = tag_set_add_tag(&tags->tags[LOCAL_TAGS], tag, key_len); + } + } + if (status) { + if (deleted) { + if (call_add) { + status->n_modified_tags++; } else { - tag_set_add_tag(&tags->tags[PROPAGATED_TAGS], tag, key_len); + status->n_deleted_tags++; } } else { - tag_set_add_tag(&tags->tags[LOCAL_TAGS], tag, key_len); + if (added) { + status->n_added_tags++; + } else { + status->n_ignored_tags++; + } } } } @@ -263,8 +283,11 @@ static void tag_set_flatten(struct tag_set *tags) { census_tag_set *census_tag_set_create(const census_tag_set *base, const census_tag *tags, int ntags, - census_tag_set_create_stats *stats) { + census_tag_set_create_status *status) { int n_invalid_tags = 0; + if (status) { + memset(status, 0, sizeof(*status)); + } census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); if (base == NULL) { memset(new_ts, 0, sizeof(census_tag_set)); @@ -280,7 +303,7 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, // ignore the tag if it is too long/short. if (key_len != 1 && key_len <= CENSUS_MAX_TAG_KV_LEN && tag->value_len <= CENSUS_MAX_TAG_KV_LEN) { - cts_add_tag(new_ts, tag, key_len); + cts_add_tag(new_ts, tag, key_len, status); } else { n_invalid_tags++; } @@ -288,12 +311,12 @@ census_tag_set *census_tag_set_create(const census_tag_set *base, tag_set_flatten(&new_ts->tags[PROPAGATED_TAGS]); tag_set_flatten(&new_ts->tags[PROPAGATED_BINARY_TAGS]); tag_set_flatten(&new_ts->tags[LOCAL_TAGS]); - if (stats != NULL) { - stats->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; - stats->n_propagated_binary_tags = + if (status != NULL) { + status->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; + status->n_propagated_binary_tags = new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; - stats->n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; - stats->n_invalid_tags = n_invalid_tags; + status->n_local_tags = new_ts->tags[LOCAL_TAGS].ntags; + status->n_invalid_tags = n_invalid_tags; } return new_ts; } @@ -480,7 +503,11 @@ static void tag_set_decode(struct tag_set *tags, const char *buffer, } census_tag_set *census_tag_set_decode(const char *buffer, size_t size, - const char *bin_buffer, size_t bin_size) { + const char *bin_buffer, size_t bin_size, + census_tag_set_create_status *status) { + if (status) { + memset(status, 0, sizeof(*status)); + } census_tag_set *new_ts = gpr_malloc(sizeof(census_tag_set)); memset(&new_ts->tags[LOCAL_TAGS], 0, sizeof(struct tag_set)); if (buffer == NULL) { @@ -493,6 +520,11 @@ census_tag_set *census_tag_set_decode(const char *buffer, size_t size, } else { tag_set_decode(&new_ts->tags[PROPAGATED_BINARY_TAGS], bin_buffer, bin_size); } + if (status) { + status->n_propagated_tags = new_ts->tags[PROPAGATED_TAGS].ntags; + status->n_propagated_binary_tags = + new_ts->tags[PROPAGATED_BINARY_TAGS].ntags; + } // TODO(aveitch): check that BINARY flag is correct for each type. return new_ts; } diff --git a/test/core/census/tag_set_test.c b/test/core/census/tag_set_test.c index 140aa8117b7..bc084ec04bd 100644 --- a/test/core/census/tag_set_test.c +++ b/test/core/census/tag_set_test.c @@ -70,7 +70,7 @@ static census_tag basic_tags[BASIC_TAG_COUNT] = { // you add or delete entries, you will also need to change the test. Other // tests that rely on specific instances have XXX_XXX_OFFSET definitions (also // change the defines below if you add/delete entires). -#define MODIFY_TAG_COUNT 10 +#define MODIFY_TAG_COUNT 11 static census_tag modify_tags[MODIFY_TAG_COUNT] = { #define REPLACE_VALUE_OFFSET 0 /* 0 */ {"key0", "replace printable", 18, 0}, // replaces tag value only @@ -88,8 +88,8 @@ static census_tag modify_tags[MODIFY_TAG_COUNT] = { /* 8 */ {"k2", (char *)&eight_byte_val, 8, CENSUS_TAG_BINARY | CENSUS_TAG_PROPAGATE}, // more flags change // non-binary -> binary - /* 9 */ {"k6", "bar", 4, - 0} // add back tag, with different value, but same length + /* 9 */ {"k6", "bar", 4, 0}, // add back tag, with different value + /* 10 */ {"foo", "bar", 4, CENSUS_TAG_PROPAGATE}, // another new tag }; // Utility function to compare tags. Returns true if all fields match. @@ -115,9 +115,12 @@ static void empty_test(void) { // Test create and iteration over basic tag set. static void basic_test(void) { + census_tag_set_create_status status; struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); + census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, &status); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); + census_tag_set_create_status expected = {2, 2, 4, 0, 8, 0, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag_set_iterator it; census_tag_set_initialize_iterator(cts, &it); census_tag tag; @@ -163,36 +166,46 @@ static void invalid_test(void) { // long keys, short value. Key lengths (including terminator) should be // <= 255 (CENSUS_MAX_TAG_KV_LEN) GPR_ASSERT(strlen(key) == 299); - struct census_tag_set *cts = census_tag_set_create(NULL, &tag, 1, NULL); + census_tag_set_create_status status; + struct census_tag_set *cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 0); + census_tag_set_create_status expected = {0, 0, 0, 0, 0, 0, 1, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag_set_destroy(cts); key[CENSUS_MAX_TAG_KV_LEN] = 0; GPR_ASSERT(strlen(key) == CENSUS_MAX_TAG_KV_LEN); - cts = census_tag_set_create(NULL, &tag, 1, NULL); + cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 0); + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag_set_destroy(cts); key[CENSUS_MAX_TAG_KV_LEN - 1] = 0; GPR_ASSERT(strlen(key) == CENSUS_MAX_TAG_KV_LEN - 1); - cts = census_tag_set_create(NULL, &tag, 1, NULL); + cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 1); + census_tag_set_create_status expected2 = {0, 0, 1, 0, 1, 0, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected2, sizeof(status)) == 0); census_tag_set_destroy(cts); // now try with long values tag.value_len = 300; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 0); + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN + 1; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 0); + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag_set_destroy(cts); tag.value_len = CENSUS_MAX_TAG_KV_LEN; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 1); + GPR_ASSERT(memcmp(&status, &expected2, sizeof(status)) == 0); census_tag_set_destroy(cts); // 0 length key. key[0] = 0; - cts = census_tag_set_create(NULL, &tag, 1, NULL); + cts = census_tag_set_create(NULL, &tag, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts) == 0); + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag_set_destroy(cts); } @@ -201,8 +214,11 @@ static void copy_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - struct census_tag_set *cts2 = census_tag_set_create(cts, NULL, 0, NULL); + census_tag_set_create_status status; + struct census_tag_set *cts2 = census_tag_set_create(cts, NULL, 0, &status); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); + census_tag_set_create_status expected = {2, 2, 4, 0, 0, 0, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); for (int i = 0; i < census_tag_set_ntags(cts2); i++) { census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key(cts2, basic_tags[i].key, &tag) == @@ -218,9 +234,12 @@ static void replace_value_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + REPLACE_VALUE_OFFSET, 1, NULL); + census_tag_set_create_status status; + struct census_tag_set *cts2 = census_tag_set_create( + cts, modify_tags + REPLACE_VALUE_OFFSET, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); + census_tag_set_create_status expected = {2, 2, 4, 0, 0, 1, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[REPLACE_VALUE_OFFSET].key, &tag) == 1); @@ -234,9 +253,12 @@ static void replace_flags_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); + census_tag_set_create_status status; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + REPLACE_FLAG_OFFSET, 1, NULL); + census_tag_set_create(cts, modify_tags + REPLACE_FLAG_OFFSET, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT); + census_tag_set_create_status expected = {1, 2, 5, 0, 0, 1, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[REPLACE_FLAG_OFFSET].key, &tag) == 1); @@ -250,9 +272,12 @@ static void delete_tag_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); + census_tag_set_create_status status; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + DELETE_TAG_OFFSET, 1, NULL); + census_tag_set_create(cts, modify_tags + DELETE_TAG_OFFSET, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT - 1); + census_tag_set_create_status expected = {2, 1, 4, 1, 0, 0, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[DELETE_TAG_OFFSET].key, &tag) == 0); @@ -265,9 +290,12 @@ static void add_tag_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); + census_tag_set_create_status status; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags + ADD_TAG_OFFSET, 1, NULL); + census_tag_set_create(cts, modify_tags + ADD_TAG_OFFSET, 1, &status); GPR_ASSERT(census_tag_set_ntags(cts2) == BASIC_TAG_COUNT + 1); + census_tag_set_create_status expected = {2, 2, 5, 0, 1, 0, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); census_tag tag; GPR_ASSERT(census_tag_set_get_tag_by_key( cts2, modify_tags[ADD_TAG_OFFSET].key, &tag) == 1); @@ -281,9 +309,12 @@ static void replace_add_delete_test(void) { struct census_tag_set *cts = census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); + census_tag_set_create_status status; struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts2) == 8); + census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, &status); + GPR_ASSERT(census_tag_set_ntags(cts2) == 9); + census_tag_set_create_status expected = {2, 1, 6, 2, 3, 4, 0, 2}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); // validate tag set contents. Use specific indices into the two arrays // holding tag values. GPR_ASSERT(validate_tag(cts2, &basic_tags[3])); @@ -294,6 +325,7 @@ static void replace_add_delete_test(void) { GPR_ASSERT(validate_tag(cts2, &modify_tags[7])); GPR_ASSERT(validate_tag(cts2, &modify_tags[8])); GPR_ASSERT(validate_tag(cts2, &modify_tags[9])); + GPR_ASSERT(validate_tag(cts2, &modify_tags[10])); GPR_ASSERT(!validate_tag(cts2, &basic_tags[0])); GPR_ASSERT(!validate_tag(cts2, &basic_tags[1])); GPR_ASSERT(!validate_tag(cts2, &basic_tags[2])); @@ -304,8 +336,8 @@ static void replace_add_delete_test(void) { census_tag_set_destroy(cts2); } -// Use the basic tag set to test encode/decode. -static void simple_encode_decode_test(void) { +// test encode/decode. +static void encode_decode_test(void) { char buf1[1000]; char buf2[1000]; struct census_tag_set *cts = @@ -317,9 +349,12 @@ static void simple_encode_decode_test(void) { GPR_ASSERT(census_tag_set_encode_propagated_binary(cts, buf2, 1) == 0); size_t b2 = census_tag_set_encode_propagated_binary(cts, buf2, 1000); GPR_ASSERT(b2 != 0); - census_tag_set *cts2 = census_tag_set_decode(buf1, b1, buf2, b2); + census_tag_set_create_status status; + census_tag_set *cts2 = census_tag_set_decode(buf1, b1, buf2, b2, &status); GPR_ASSERT(cts2 != NULL); GPR_ASSERT(census_tag_set_ntags(cts2) == 4); + census_tag_set_create_status expected = {2, 2, 0, 0, 0, 0, 0, 0}; + GPR_ASSERT(memcmp(&status, &expected, sizeof(status)) == 0); for (int i = 0; i < census_tag_set_ntags(cts); i++) { census_tag tag; if (CENSUS_TAG_IS_PROPAGATED(basic_tags[i].flags)) { @@ -335,31 +370,6 @@ static void simple_encode_decode_test(void) { census_tag_set_destroy(cts); } -// Use more complex/modified tag set to test encode/decode. -static void complex_encode_decode_test(void) { - char buf1[500]; - char buf2[500]; - struct census_tag_set *cts = - census_tag_set_create(NULL, basic_tags, BASIC_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts) == BASIC_TAG_COUNT); - struct census_tag_set *cts2 = - census_tag_set_create(cts, modify_tags, MODIFY_TAG_COUNT, NULL); - GPR_ASSERT(census_tag_set_ntags(cts2) == 8); - - size_t b1 = census_tag_set_encode_propagated(cts2, buf1, 500); - GPR_ASSERT(b1 != 0); - size_t b2 = census_tag_set_encode_propagated_binary(cts2, buf2, 500); - GPR_ASSERT(b2 != 0); - census_tag_set *cts3 = census_tag_set_decode(buf1, b1, buf2, b2); - GPR_ASSERT(cts3 != NULL); - GPR_ASSERT(census_tag_set_ntags(cts3) == 2); - GPR_ASSERT(validate_tag(cts3, &basic_tags[4])); - GPR_ASSERT(validate_tag(cts3, &modify_tags[8])); - census_tag_set_destroy(cts3); - census_tag_set_destroy(cts2); - census_tag_set_destroy(cts); -} - int main(int argc, char *argv[]) { grpc_test_init(argc, argv); empty_test(); @@ -372,7 +382,6 @@ int main(int argc, char *argv[]) { delete_tag_test(); add_tag_test(); replace_add_delete_test(); - simple_encode_decode_test(); - complex_encode_decode_test(); + encode_decode_test(); return 0; }