CIRC-9456 - Implicit Tag Support #833

RobBoeckermann · 2023-01-19T00:18:23Z

This PR adds the same implicit tag support as before:

Allow implicit tags with lengths up to 4103 (the maximum length of a __name tag pair).
Convert arrays to dynamic buffers so that they can be expanded to support the length of implicit tags when needed.
Allow filtering do be done on __name even if the max number of stream tags are already in used.

but has been reworked to:
4. Allow adding multiple implicit tags to the same tagset.
5. Allow adding multiple tags at the same time.

noit_metric_add_implicit_tags_to_tagset can be passed a *char containing a comma separated list of implicit tags and can now optionally be used to get the canonical name as well.

I refactored to keep code as DRY as possible by only maintaining one function, add_tags_to_tagset_builder, that can be used to add "one" or "many" tags to a tagset (instead of having two different functions for this logic).
The add_one and add many endpoints have the same behavior, but are kept as separate functions to prevent altering public function signatures.

src/noit_metric_tag_search.c

pjulien

Please confirm you ran all the tests with ASAN
It's also worth nothing that C++ has been added to libmtev, this means the barrier to C++ has been removed from here as well

src/noit_filters.c

src/noit_metric.c

src/noit_metric_tag_search.c

test/test_tags.c

src/noit_filters.c

src/noit_message_decoder.c

RobBoeckermann · 2023-01-24T18:24:20Z

Please confirm you ran all the tests with ASAN

all tests are passing on an asan build. the only warnings are the leaks we are discussing that I believe belong to libmtev.

It's also worth nothing that C++ has been added to libmtev, this means the barrier to C++ has been removed from here as well

does this mean we can add c++ code in libmtev if/when we need it? is there a way for me to take advantage of this in this ticket?

pjulien · 2023-01-24T18:35:39Z

all tests are passing on an asan build. the only warnings are the leaks we are discussing that I believe belong to libmtev.

that does not look real to me

It's also worth nothing that C++ has been added to libmtev, this means the barrier to C++ has been removed from here as well

does this mean we can add c++ code in libmtev if/when we need it? is there a way for me to take advantage of this in this ticket?

Libmtev probably not, have to keep ABI and source compatibility, but it means it can be used from reconnoiter

test/test_tags.cpp

src/noit_message_decoder.c

src/noit_metric_tag_search.c

src/noit_check.c

test/test_tags.cpp

src/noit_message_decoder.c

src/noit_metric_tag_search.c

RobBoeckermann · 2023-04-27T16:13:32Z

these reconnoiter changes are breaking the picker tests on master:

[ RUN      ] matching/basic_matching/basic_matching_spec.lua @ 79: correctly filter input based on rules verifies that we matched the numeric ruleset
/opt/circonus/libexec/mtev/lua/mtev/HttpClient.lua:167: no response

stack traceback:
        /opt/circonus/libexec/mtev/lua/mtev/HttpClient.lua:167: in function 'get_headers'
        /opt/circonus/libexec/mtev/lua/mtev/HttpClient.lua:324: in function 'get_response'
        /opt/circonus/libexec/mtev/lua/mtevbusted/api.lua:129: in function 'load_data'
        matching/basic_matching/basic_matching_spec.lua:80: in function <matching/basic_matching/basic_matching_spec.lua:79>

[  ERROR   ] matching/basic_matching/basic_matching_spec.lua @ 79: correctly filter input based on rules verifies that we matched the numeric ruleset (29.53 ms)

This won't be merged until this is resolved.

keep code as DRY as possible without changing public API signatures by only maintaining one function, `add_tags_to_tagset_builder`, that can add either one or many tags to a tagset. the `add_one` and `add many` endpoints have the same behavior, but are kept as separate functions to prevent altering function signatures. allow optionally getting canonical name when using `noit_metric_add_implicit_tags_to_tagset`. add tests for implicit tags.

… realloc memory that was never allocated with malloc

… time

properly initialize tagset `tagset_implicit->tags` was being set through `MKTAGSETCOPY`. we do not need to copy the tags for `tagset_implicit` because it is an empty set and we are not using anything from `id` that could be changed. use `calloc` if memory has not been allocated already.

.tag needs to be given it's own memory block so that it persists in the tagset after the input goes out of scope. +1 to add a null terminator

the previous realloc was always for the same size that we just calloced.

This reverts commit 2787251.

This reverts commit 36f9db3.

…lass_t`. add implicit tags to existing tagsets, but increase the max size to allow for them. we can increase the max size because these tagsets are copied locally and the extended size does not conflict with the functions performed on the tagset in this scope. since it is a copy, the implicit tags are not being added to the id's actual check and stream tags.

no need to get the encode(decode) length because they are inverse operations

… places

this fixes a segfault when attempting to realloc this memory later. this also allows us to allocate only the necessary memory instead of the max.

RobBoeckermann · 2023-06-21T18:16:45Z

@pjulien there is a bug in a picker test when using these changes to libnoit:

matching/basic_matching/basic_matching_spec.lua @ 80: correctly filter input based on rules verifies that we matched the numeric ruleset
matching/basic_matching/basic_matching_spec.lua:86: Expected objects to be equal.
Passed in:
(number) 0
Expected:
(number) 1

This is failing because the debug/matching log:

[2023-06-20 20:13:56.996525] [debug/matching] [t@26651/e:default/2,integrations/selector/selector_integration1: b52856c0-6eb6-44e6-e016-c2794bae6b73:a_numeric_metric forwarded to numeric_ruleset_test_exchange:numeric_ruleset_test_route

is not found in the log file.

This log is written by the picker's selector_integration.cpp:168, but this code is not being hit because, higher in the callstack, a while loop on picker's metrics.cpp:156 is not being entered due to noit_metric_director_lane_next is returning nullptr.

This is nullptr because the metric director's fifo is empty, so ck_fifo_spsc_dequeue is unable to return a pointer.

I'm not sure yet why the fifo is not being added to, but am continuing to look into it.

esvc

pjulien

can you run try to run this under lsan/asan please?

RobBoeckermann requested review from pjulien, pamaddox, mcieslinski and vasuraman11 January 19, 2023 00:18

RobBoeckermann commented Jan 19, 2023

View reviewed changes

src/noit_metric_tag_search.c Outdated Show resolved Hide resolved

pjulien reviewed Jan 19, 2023

View reviewed changes

pjulien reviewed Jan 23, 2023

View reviewed changes

src/noit_filters.c Outdated Show resolved Hide resolved

src/noit_filters.c Outdated Show resolved Hide resolved

src/noit_filters.c Outdated Show resolved Hide resolved

src/noit_message_decoder.c Show resolved Hide resolved

pjulien reviewed Jan 24, 2023

View reviewed changes

test/test_tags.cpp Outdated Show resolved Hide resolved

RobBoeckermann force-pushed the ci-CIRC-9456.implicit_tag_support.rob branch from d86cd4e to e398956 Compare March 20, 2023 16:51

pjulien reviewed Apr 10, 2023

View reviewed changes

src/noit_message_decoder.c Outdated Show resolved Hide resolved

src/noit_message_decoder.c Outdated Show resolved Hide resolved

pjulien reviewed Apr 10, 2023

View reviewed changes

src/noit_message_decoder.c Outdated Show resolved Hide resolved

RobBoeckermann commented Apr 21, 2023

View reviewed changes

src/noit_message_decoder.c Show resolved Hide resolved

RobBoeckermann commented Apr 21, 2023

View reviewed changes

src/noit_metric_tag_search.c Show resolved Hide resolved

pjulien reviewed Apr 21, 2023

View reviewed changes

RobBoeckermann force-pushed the ci-CIRC-9456.implicit_tag_support.rob branch 2 times, most recently from 2d85305 to 7ca364c Compare April 25, 2023 20:23

RobBoeckermann requested a review from pjulien April 26, 2023 14:15

pjulien previously approved these changes Apr 26, 2023

View reviewed changes

RobBoeckermann force-pushed the ci-CIRC-9456.implicit_tag_support.rob branch from 7ca364c to d025d2d Compare May 15, 2023 23:16

RobBoeckermann force-pushed the ci-CIRC-9456.implicit_tag_support.rob branch from c3d106e to 2831e01 Compare May 30, 2023 20:45

RobBoeckermann added 7 commits June 20, 2023 21:30

implicit tag implementation

e57418e

fix bug preventing adding tags to existing tagset due to inability to…

a8c8f00

… realloc memory that was never allocated with malloc

reduce calls to builder functions by adding multiple tags at the same…

dbfaf75

… time

test el7 failure

bfbe5f0

revert logging from last commit. error is no longer occurring

f4a5653

convert test_tags.c to test_tags.cpp

3ee3273

RobBoeckermann and others added 24 commits June 20, 2023 21:30

use noit_metric_tagset_cleanup for tagsets

aec6bb7

properly initialize and clean tagsets

506fad4

fix testing leaks

ee46b9f

formatting

5b14a1f

noit_metric_tag_t must have their members copied individually.

8ecc00a

.tag needs to be given it's own memory block so that it persists in the tagset after the input goes out of scope. +1 to add a null terminator

use parsed_tag_count instead of tag_count in case they differ.

d0da8df

the previous realloc was always for the same size that we just calloced.

cast char* to void*

9ebf3e8

Revert "cast char* to void*"

5922d6e

This reverts commit 2787251.

Revert "noit_metric_tag_t must have their members copied individually."

839fcdd

This reverts commit 36f9db3.

formatting

9ede60f

generate tag pairs and assert their lengths

ca5d1f8

don't init out arg

43ea246

"not" is reserved. use different name to avoid use of _.

62fb17f

pass string by reference

f3b38f0

properly init tagsets

bd7fedd

const

0b01bd4

use fmt::format

82d4c48

add circonus-platform-library-fmt dependency

40663c3

+1 to ensured length to account for for colon added to cat-only tags

303a7cd

no need to get the encode(decode) length because they are inverse operations

call base64 functions instead of defining identical logic in multiple…

13b7ea7

… places

dynamically allocate memory

1a4e727

this fixes a segfault when attempting to realloc this memory later. this also allows us to allocate only the necessary memory instead of the max.

free calloced memory

00c6540

RobBoeckermann force-pushed the ci-CIRC-9456.implicit_tag_support.rob branch from 445e97f to 00c6540 Compare June 20, 2023 23:00

Use the c++ extern "C" to properly link C functions to a cpp file

4efaf6d

get string length instead of array size

ae0990a

pjulien reviewed Jun 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CIRC-9456 - Implicit Tag Support #833

CIRC-9456 - Implicit Tag Support #833

RobBoeckermann commented Jan 19, 2023

pjulien left a comment

RobBoeckermann commented Jan 24, 2023

pjulien commented Jan 24, 2023

RobBoeckermann commented Apr 27, 2023

RobBoeckermann commented Jun 21, 2023 •

edited by pjulien

pjulien left a comment

CIRC-9456 - Implicit Tag Support #833

Are you sure you want to change the base?

CIRC-9456 - Implicit Tag Support #833

Conversation

RobBoeckermann commented Jan 19, 2023

pjulien left a comment

Choose a reason for hiding this comment

RobBoeckermann commented Jan 24, 2023

pjulien commented Jan 24, 2023

RobBoeckermann commented Apr 27, 2023

RobBoeckermann commented Jun 21, 2023 • edited by pjulien

pjulien left a comment

Choose a reason for hiding this comment

RobBoeckermann commented Jun 21, 2023 •

edited by pjulien