Make sure string metrics are dumped in RecDumpRecords#13074
Make sure string metrics are dumped in RecDumpRecords#13074cmcfarlen wants to merge 1 commit intoapache:masterfrom
Conversation
|
[approve ci autest 1] |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the records dump path so plugin string metrics (e.g., used by stats_over_http) are included when RecDumpRecords / TSRecordDump enumerates metrics.
Changes:
- Extend
RecDumpRecords()to also emitts::Metrics::StaticStringentries asRECT_PLUGIN/ string data. - Add a Catch2 unit test that registers a
StaticStringmetric and asserts it appears inRecDumpRecords()output. - Wire the new unit test into the
test_recordstarget.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/records/RecCore.cc |
Adds dumping of static string metrics via StaticString::for_each() into RecDumpRecords(). |
src/records/unit_tests/test_RecDumpRecords.cc |
New unit test validating string metrics appear in RecDumpRecords() output. |
src/records/CMakeLists.txt |
Adds the new unit test source to the test_records executable. |
| } else if (data_type == RECD_INT || data_type == RECD_COUNTER) { | ||
| entry.int_value = datum->rec_int; |
There was a problem hiding this comment.
collect_callback() reads datum->rec_int for RECD_COUNTER entries. Since RecData is a union, counters should be read from datum->rec_counter when data_type == RECD_COUNTER (reading a non-active union member is undefined behavior and can trip UBSan even if the value isn't later asserted in this test).
| } else if (data_type == RECD_INT || data_type == RECD_COUNTER) { | |
| entry.int_value = datum->rec_int; | |
| } else if (data_type == RECD_INT) { | |
| entry.int_value = datum->rec_int; | |
| } else if (data_type == RECD_COUNTER) { | |
| entry.int_value = datum->rec_counter; |
| ts::Metrics::StaticString::instance().for_each([&](const std::string &name, const std::string &value) { | ||
| datum.rec_string = const_cast<char *>(value.c_str()); | ||
|
|
||
| callback(RECT_PLUGIN, edata, true, name.data(), TS_RECORDDATATYPE_STRING, &datum); | ||
| }); |
There was a problem hiding this comment.
ts::Metrics::StaticString::for_each() invokes its callback while holding the internal mutex (per Metrics.h). Calling RecDumpRecords()'s external callback(...) under that lock risks deadlocks / long lock hold times if the callback does anything non-trivial. Consider snapshotting the (name,value) pairs while holding the mutex, then invoking callback(...) after releasing it.
This bug was preventing string metrics from showing up in plugins like stats_over_http.