Skip to content

Fix test_prometheus_metrics-t to handle Prometheus labels #5380

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.

Sign up for GitHub

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

Open
renecannao wants to merge 1 commit into
base: v3.0
Choose a base branch
from
Open

Fix test_prometheus_metrics-t to handle Prometheus labels #5380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion test/tap/tap/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,7 +1855,13 @@ map<string, double> parse_prometheus_metrics(const string& s) {
for (const string ln : lines) {
if (ln.empty() == false && ln[0] != '#') {
pair<string, string> p_line_val { split_line_by_last(ln, ' ') };
metrics_map.insert({p_line_val.first, stod(p_line_val.second)});
// Strip labels from metric name (e.g., "metric{label=\"value\"}" -> "metric")
string metric_name = p_line_val.first;
size_t brace_pos = metric_name.find('{');
if (brace_pos != string::npos) {
metric_name = metric_name.substr(0, brace_pos);
}
metrics_map.insert({metric_name, stod(p_line_val.second)});
Comment on lines +1859 to +1864
Copy link

@gemini-code-assist gemini-code-assist bot Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic to strip labels from the metric name can be made more concise. You can combine std::string::substr with std::string::find in a single expression. When find doesn't locate the character, it returns std::string::npos, and substr(0, std::string::npos) correctly returns a copy of the entire string. This simplifies the code by removing the conditional logic and intermediate variables.

            metrics_map.insert({p_line_val.first.substr(0, p_line_val.first.find('{')), stod(p_line_val.second)});

Comment on lines +1858 to +1864
Copy link

@coderabbitai coderabbitai bot Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find where parse_prometheus_metrics is called
rg -n 'parse_prometheus_metrics' --type=cpp -B2 -A2

Repository: sysown/proxysql

Length of output: 1737

🏁 Script executed:

# Find test files that might consume this function
fd -e cpp -e hpp | head -20

Repository: sysown/proxysql

Length of output: 640

🏁 Script executed:

# Look for Prometheus metrics output in tests
rg -n 'prometheus\|metrics_map' --type=cpp -B2 -A2 | head -100

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Look at test_prometheus_metrics-t.cpp to see how metrics_vals is used
cat test/tap/tests/test_prometheus_metrics-t.cpp | head -100

Repository: sysown/proxysql

Length of output: 2862

🏁 Script executed:

# Look at reg_test_4556-ssl_error_queue-t.cpp to see how m_id_val is used
rg -n 'const map.*m_id_val|m_id_val\[' test/tap/tests/reg_test_4556-ssl_error_queue-t.cpp -A3

Repository: sysown/proxysql

Length of output: 261

🏁 Script executed:

# Search for any test data or fixtures that contain Prometheus metrics format
rg -n 'metric{|protocol=|label=' test/tap/tests/ --type=cpp -C2

Repository: sysown/proxysql

Length of output: 172

🏁 Script executed:

# Search for Prometheus metrics with labels in test files
rg 'proxysql_.*{' test/tap/tests/ --type=cpp -C2

Repository: sysown/proxysql

Length of output: 146

🏁 Script executed:

# Look at what the Prometheus output looks like - search for "# HELP\|# TYPE"
rg 'HELP|TYPE' test/tap/tests/ --type=cpp -C2 | head -50

Repository: sysown/proxysql

Length of output: 3686

🏁 Script executed:

# Check the actual usage of metrics_vals after parsing
rg -n 'metrics_vals\.find\|metrics_vals\[' test/tap/tests/test_prometheus_metrics-t.cpp -A2 -B2

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Look for any fixtures or test data files that might have prometheus output
find test/tap -name "*prometheus*" -o -name "*metrics*" | grep -v '\\.o$'

Repository: sysown/proxysql

Length of output: 273

🏁 Script executed:

# Look at parse_prometheus_metrics implementation completely
sed -n '1851,1870p' test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 711

🏁 Script executed:

# Look for how metrics are looked up - they search for base metric names
rg 'proxysql_' test/tap/tests/test_prometheus_metrics-t.cpp | head -20

Repository: sysown/proxysql

Length of output: 1671

🏁 Script executed:

# Look at the disabled test which might have test data
head -150 test/tap/tests/test_admin_prometheus_metrics_dump-t_disabled_gh3571.cpp

Repository: sysown/proxysql

Length of output: 2674

🏁 Script executed:

# Search for actual metric values in the test to understand structure
rg -n 'find\(' test/tap/tests/test_prometheus_metrics-t.cpp -A1 -B1 | head -40

Repository: sysown/proxysql

Length of output: 1787

🏁 Script executed:

# Check if extract_metric_tags is called and used with the returned map from parse_prometheus_metrics
rg -n 'extract_metric_tags\(' test/tap/tests/test_prometheus_metrics-t.cpp -A2 -B2

Repository: sysown/proxysql

Length of output: 1165

🏁 Script executed:

# Look at the full extract_metric_tags function
rg -n 'extract_metric_tags' test/tap/tap/utils.cpp -A15

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Check what the actual Prometheus output format is - look at the comment/example
rg -n 'HELP\|# TYPE' test/tap/tap/utils.cpp -B2 -A2

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Look at the complete extract_metric_tags function to see what it returns when labels are missing
sed -n '277,310p' test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 1012

🏁 Script executed:

# Check what happens when extract_metric_tags finds no labels - does it return empty or what?
rg -n 'extract_metric_tags' test/tap/tap/utils.cpp -A20 | head -40

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Look at the specific line where metric_tags.at("hostgroup") is called
sed -n '360,370p' test/tap/tests/test_prometheus_metrics-t.cpp

Repository: sysown/proxysql

Length of output: 489

🏁 Script executed:

# Look at the complete extract_metric_tags implementation to confirm what it returns
sed -n '277,300p' test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 791

🏁 Script executed:

# Check if there's any initialization or default behavior after find returns npos
sed -n '275,310p' test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 1096

🏁 Script executed:

# Use ast-grep to find the extract_metric_tags function definition
ast-grep --pattern $'map<string,string> extract_metric_tags($_) {
  $$$
}'

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Look at utils.cpp around line 277 where extract_metric_tags should be
head -320 test/tap/tap/utils.cpp | tail -50

Repository: sysown/proxysql

Length of output: 1546

🏁 Script executed:

# Search for the exact location of extract_metric_tags function
rg -n 'map<string,string> extract_metric_tags' test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Once we know the line number, read that section
rg -n 'map<string,string> extract_metric_tags' test/tap/tap/utils.cpp -A25

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Search for extract_metric_tags without the full signature
rg -n 'extract_metric_tags' test/tap/tap/utils.cpp | head -5

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Look at the line numbers from earlier results - they showed line 277 has it
sed -n '277,320p' test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 1362

🏁 Script executed:

# Try searching in the header file
rg -n 'extract_metric_tags' test/tap/tap/utils.h -A10

Repository: sysown/proxysql

Length of output: 41

🏁 Script executed:

# Search for it anywhere in the tap directory
find test/tap -name "*.cpp" -o -name "*.h" | xargs grep -l 'extract_metric_tags'

Repository: sysown/proxysql

Length of output: 103

🏁 Script executed:

# Get file size and search more carefully
wc -l test/tap/tap/utils.cpp

Repository: sysown/proxysql

Length of output: 86

🏁 Script executed:

# Search for extract_metric_tags in the test file
rg -n 'extract_metric_tags' test/tap/tests/test_prometheus_metrics-t.cpp -B2 -A15

Repository: sysown/proxysql

Length of output: 2974

🏁 Script executed:

# Also check if extract_metric_tags is defined in the test file or just used
grep -n 'extract_metric_tags' test/tap/tests/test_prometheus_metrics-t.cpp

Repository: sysown/proxysql

Length of output: 373

🏁 Script executed:

# Look for the function definition in test_prometheus_metrics-t.cpp
sed -n '250,330p' test/tap/tests/test_prometheus_metrics-t.cpp

Repository: sysown/proxysql

Length of output: 2511

🏁 Script executed:

# Confirm the call to metric_tags.at("hostgroup") on line 363
# If extract_metric_tags returns empty map (because labels are stripped), 
# .at("hostgroup") will throw std::out_of_range exception
sed -n '360,365p' test/tap/tests/test_prometheus_metrics-t.cpp

Repository: sysown/proxysql

Length of output: 330

🏁 Script executed:

# Check if proxysql_connpool_conns metric actually has hostgroup labels in Prometheus output
# Look for any documentation or comments about metric format
rg -n 'proxysql_connpool_conns' test/tap/ -C3 | head -40

Repository: sysown/proxysql

Length of output: 3590

🏁 Script executed:

# Check the git log to see what the original code did before the labels were stripped
git log --oneline test/tap/tap/utils.cpp | head -5

Repository: sysown/proxysql

Length of output: 125

🏁 Script executed:

# Look at the commit that mentions handling Prometheus labels
git show 195bbfb5 --stat

Repository: sysown/proxysql

Length of output: 50371

🏁 Script executed:

# Check what changed in that commit for parse_prometheus_metrics
git show 195bbfb5 test/tap/tap/utils.cpp | head -80

Repository: sysown/proxysql

Length of output: 2745

🏁 Script executed:

# Verify that extract_metric_tags is actually called on the parsed metrics
# and will fail when labels are stripped
grep -n 'extract_metric_tags' test/tap/tests/test_prometheus_metrics-t.cpp

Repository: sysown/proxysql

Length of output: 373

🏁 Script executed:

# Look at what extract_metric_tags returns when it doesn't find '{'
sed -n '277,290p' test/tap/tests/test_prometheus_metrics-t.cpp

Repository: sysown/proxysql

Length of output: 560

This change breaks test assertions by stripping labels that extract_metric_tags() requires.

parse_prometheus_metrics() now strips all labels from metric names, but the test code at lines 362–363 calls extract_metric_tags(p_metric_val.first) expecting the labels to be present. Since extract_metric_tags() searches for { in the metric name and returns an empty map when none is found, the subsequent .at("hostgroup") call will throw std::out_of_range, causing test failures.

The same issue occurs at lines 466 and 640 where other tests call extract_metric_tags() on the returned metrics map expecting labels to be intact.

Additionally, if any Prometheus metric is emitted with multiple label variants (e.g., proxysql_connpool_conns{hostgroup="0"} 5 and proxysql_connpool_conns{hostgroup="1"} 10), map::insert() silently drops all but the first occurrence, losing data.

The label-stripping approach conflicts with the test expectations. Consider either preserving the full metric names with labels in the returned map or refactoring the tests to work without label information.

🤖 Prompt for AI Agents 
In `@test/tap/tap/utils.cpp` around lines 1858 - 1864, parse_prometheus_metrics()
currently strips labels from metric names and uses map::insert which drops
duplicate label variants, breaking tests that call extract_metric_tags() on the
returned keys; revert to preserving the full metric key (use p_line_val.first
directly instead of stripping into metric_name) and replace the insert call with
an assignment/emplace that records every variant (e.g., use
metrics_map[p_line_val.first] = stod(p_line_val.second) or a multimap/vector if
you need to keep multiple values) so extract_metric_tags() can find the '{' and
labels and duplicates are not silently lost.

}
}

Expand Down
Loading