-
Notifications
You must be signed in to change notification settings - Fork 62
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.
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
CIRC-9456 - Implicit Tag Support #833
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
all tests are passing on an asan build. the only warnings are the leaks we are discussing that I believe belong to libmtev.
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? |
that does not look real to me
Libmtev probably not, have to keep ABI and source compatibility, but it means it can be used from reconnoiter |
d86cd4e
to
e398956
Compare
2d85305
to
7ca364c
Compare
these reconnoiter changes are breaking the picker tests on master:
This won't be merged until this is resolved. |
7ca364c
to
d025d2d
Compare
c3d106e
to
2831e01
Compare
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
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
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.
445e97f
to
00c6540
Compare
@pjulien there is a bug in a picker test when using these changes to libnoit:
This is failing because the
is not found in the log file. This log is written by the picker's This is I'm not sure yet why the fifo is not being added to, but am continuing to look into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you run try to run this under lsan/asan please?
This PR adds the same implicit tag support as before:
__name
tag pair).__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
andadd many
endpoints have the same behavior, but are kept as separate functions to prevent altering public function signatures.