[PWCI] "telemetry: fix adding dicts in container arrays"#603
Open
[PWCI] "telemetry: fix adding dicts in container arrays"#603
Conversation
Currently, adding TEL_DICT containers in TEL_ARRAY_CONTAINER containers is not supported by rte_tel_data_add_array_container. It wrongfully returns -EINVAL where it should allow it. This is supported by the JSON formatter. Allow the same value types than rte_tel_data_add_dict_container(). Fixes: c933bb5 ("telemetry: support array values in data object") Signed-off-by: Robin Jarry <rjarry@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideExtends telemetry array-container validation to allow nested dictionary-type telemetry data as entries in container arrays while preserving existing type and bounds checks. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughExtends the validation logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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 @lib/telemetry/telemetry_data.c:
- Around line 99-102: The change in rte_tel_data_add_array_container now permits
TEL_DICT as an array value type but there is no unit test covering an array
whose elements are TEL_DICT; add a test in app/test/test_telemetry_data.c that
constructs an array container via rte_tel_data_add_array_container with
val->type == TEL_DICT (use the same creation helpers used for existing array
tests), insert one or more dict elements into the array, call
container_to_json() (or the public serialization path exercised by other tests)
and assert the produced JSON matches the expected nested structure to ensure the
JSON formatter handles TEL_DICT-in-array cases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/telemetry/telemetry_data.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
Comment on lines
+99
to
+102
| if (d->type != TEL_ARRAY_CONTAINER || (val->type != TEL_ARRAY_UINT | ||
| && val->type != TEL_ARRAY_INT | ||
| && val->type != TEL_ARRAY_STRING)) | ||
| && val->type != TEL_ARRAY_STRING | ||
| && val->type != TEL_DICT)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for JSON formatter code that handles array containers ==="
rg -n -C 3 'TEL_ARRAY_CONTAINER' --type c
echo -e "\n=== Searching for test files related to telemetry array containers ==="
fd -e c test.*telemetry 2>/dev/null || echo "No matches for test.*telemetry"
fd -e c telemetry.*test 2>/dev/null || echo "No matches for telemetry.*test"
fd -type f -name "*test*" -path "*/telemetry/*" 2>/dev/null | head -20
echo -e "\n=== Checking for existing tests of rte_tel_data_add_array_container ==="
rg -n 'rte_tel_data_add_array_container' --type cRepository: ovsrobot/dpdk
Length of output: 3959
🏁 Script executed:
#!/bin/bash
echo "=== Examining test cases that use rte_tel_data_add_array_container ==="
sed -n '155,170p' app/test/test_telemetry_data.c
echo -e "\n=== Checking what types child_data and child_data2 are ==="
sed -n '140,165p' app/test/test_telemetry_data.c
echo -e "\n=== Looking at JSON formatter processing of array containers (lines 346-360) ==="
sed -n '346,370p' lib/telemetry/telemetry.c
echo -e "\n=== Searching for TEL_DICT handling in JSON formatter ==="
rg -n 'TEL_DICT' lib/telemetry/telemetry.c -C 3
echo -e "\n=== Checking if rte_tel_data_add_dict_container exists and what it does ==="
rg -n -A 10 'rte_tel_data_add_dict_container' lib/telemetry/telemetry_data.c | head -40Repository: ovsrobot/dpdk
Length of output: 3644
LGTM. Change correctly aligns validation with JSON formatter and existing dict container function.
The modification appropriately extends rte_tel_data_add_array_container to accept TEL_DICT alongside existing array types, making it consistent with rte_tel_data_add_dict_container (lines 232-235). The JSON formatter's container_to_json() function already recursively processes all container types, including TEL_DICT, so the infrastructure is in place.
Note: Existing tests in app/test/test_telemetry_data.c verify array containers with array value types, but don't explicitly test the newly-enabled TEL_DICT as a value type within array containers. Consider adding a test case for this scenario.
🤖 Prompt for AI Agents
In @lib/telemetry/telemetry_data.c around lines 99 - 102, The change in
rte_tel_data_add_array_container now permits TEL_DICT as an array value type but
there is no unit test covering an array whose elements are TEL_DICT; add a test
in app/test/test_telemetry_data.c that constructs an array container via
rte_tel_data_add_array_container with val->type == TEL_DICT (use the same
creation helpers used for existing array tests), insert one or more dict
elements into the array, call container_to_json() (or the public serialization
path exercised by other tests) and assert the produced JSON matches the expected
nested structure to ensure the JSON formatter handles TEL_DICT-in-array cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: This is an auto submission for "telemetry: fix adding dicts in container arrays".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36982" for details.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.