Fix test_prometheus_metrics-t to handle Prometheus labels#5380
Fix test_prometheus_metrics-t to handle Prometheus labels#5380renecannao wants to merge 1 commit intov3.0from
Conversation
Update parse_prometheus_metrics() to strip labels from metric names
(e.g., 'metric{protocol="mysql"}' -> 'metric') so tests can use
simple metric names without labels.
Fixes #5378
Summary of ChangesHello @renecannao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughModified the Prometheus metrics parser to extract metric names without their label sets before storing them in the metrics map. The parser now strips inline labels (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly updates the parse_prometheus_metrics function to strip labels from metric names, allowing tests to use simple metric names. The implementation is correct and addresses the issue. I've added one suggestion to make the code more concise and improve maintainability.
| 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)}); |
There was a problem hiding this comment.
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)});
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/tap/tap/utils.cpp`:
- Around line 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.
🧹 Nitpick comments (1)
test/tap/tap/utils.cpp (1)
1855-1855: Avoid unnecessary string copy in range-based for loop.
lnis copied on each iteration. Use aconstreference instead.Suggested fix
- for (const string ln : lines) { + for (const string& ln : lines) {
| // 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)}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where parse_prometheus_metrics is called
rg -n 'parse_prometheus_metrics' --type=cpp -B2 -A2Repository: sysown/proxysql
Length of output: 1737
🏁 Script executed:
# Find test files that might consume this function
fd -e cpp -e hpp | head -20Repository: 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 -100Repository: 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 -100Repository: 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 -A3Repository: 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 -C2Repository: sysown/proxysql
Length of output: 172
🏁 Script executed:
# Search for Prometheus metrics with labels in test files
rg 'proxysql_.*{' test/tap/tests/ --type=cpp -C2Repository: 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 -50Repository: 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 -B2Repository: 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.cppRepository: 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 -20Repository: 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.cppRepository: 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 -40Repository: 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 -B2Repository: 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 -A15Repository: 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 -A2Repository: 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.cppRepository: 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 -40Repository: 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.cppRepository: 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.cppRepository: 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.cppRepository: 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 -50Repository: 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.cppRepository: 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 -A25Repository: 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 -5Repository: 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.cppRepository: sysown/proxysql
Length of output: 1362
🏁 Script executed:
# Try searching in the header file
rg -n 'extract_metric_tags' test/tap/tap/utils.h -A10Repository: 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.cppRepository: 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 -A15Repository: 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.cppRepository: 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.cppRepository: 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.cppRepository: 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 -40Repository: 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 -5Repository: sysown/proxysql
Length of output: 125
🏁 Script executed:
# Look at the commit that mentions handling Prometheus labels
git show 195bbfb5 --statRepository: 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 -80Repository: 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.cppRepository: 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.cppRepository: 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.
|
retest this please |
2 participants
Update parse_prometheus_metrics() to strip labels from metric names
(e.g., 'metric{protocol="mysql"}' -> 'metric') so tests can use
simple metric names without labels.
Fixes #5378
Summary by CodeRabbit