circonus-labs / reconnoiter

Large-scale Monitoring and Trend Analysis System
Other
245 stars 62 forks source link

CIRC-9456 - Implicit Tag Support #833

Open RobBoeckermann opened 1 year ago

RobBoeckermann commented 1 year ago

This PR adds the same implicit tag support as before:

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

but has been reworked to:

  1. Allow adding multiple implicit tags to the same tagset.
  2. 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.

RobBoeckermann commented 1 year ago
  • 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 commented 1 year ago

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

RobBoeckermann commented 1 year ago

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.

RobBoeckermann commented 1 year ago

@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.