test(tracing): add control plane sources attributes tests#923
Open
test(tracing): add control plane sources attributes tests#923
Conversation
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds documentation and local config for Jaeger tracing; introduces Make variables and targets to configure tracing for Istio and Kuadrant during local setup; extends tracing clients to accept a microsecond Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant Makefile
participant Kubernetes as K8s
participant Istio
participant KuadrantOperator as Kuadrant
participant ToolsNS as Tools
Developer->>Makefile: make local-setup (INSTALL_TRACING=true)
Makefile->>Makefile: deploy-testsuite-tools --namespace $(TOOLS_NAMESPACE)
Makefile->>K8s: helm install Jaeger into ToolsNS
Makefile->>Istio: configure-istio-tracing (patch CR, apply Telemetry)
Istio-->>K8s: update istio-system resources
Makefile->>Kuadrant: configure-kuadrant-tracing-operator (set OTEL env, log level)
Kuadrant-->>K8s: rollout patch on kuadrant-operator-controller-manager
Makefile->>Kuadrant: configure-kuadrant-tracing-cr (patch Kuadrant CR observability)
Kuadrant-->>K8s: update Kuadrant resources
K8s-->>Developer: tracing components deployed and configured
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (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 |
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
CLAUDE.md (1)
94-101: Consider removing specific test counts to avoid staleness.The comments mentioning "40 tests" and "10 tests" will become outdated as tests are added or removed. Consider removing these counts or making them more generic.
♻️ Suggested change
4. **Run tracing tests:** ```bash - # Control plane tracing tests (40 tests) + # Control plane tracing tests make testsuite/tests/singlecluster/tracing/control_plane/ - # Data plane tracing tests (10 tests) + # Data plane tracing tests make testsuite/tests/singlecluster/tracing/data_plane_tracing/ ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 94 - 101, The documentation comments include hard-coded test counts ("40 tests" and "10 tests") that can become stale; remove the numeric counts from the two comment lines above the make commands (the lines commenting "Control plane tracing tests (40 tests)" and "Data plane tracing tests (10 tests)") so they read generically as "Control plane tracing tests" and "Data plane tracing tests" next to the make commands `make testsuite/tests/singlecluster/tracing/control_plane/` and `make testsuite/tests/singlecluster/tracing/data_plane_tracing/`.make/istio.mk (1)
36-36: Consider extracting the long JSON patch for improved maintainability.This single-line JSON patch is difficult to read and maintain. Additionally, the service name and port (4318) are hardcoded here rather than being derived from
$(JAEGER_COLLECTOR_ENDPOINT)defined inmake/vars.mk, which could lead to inconsistencies if the endpoint changes.♻️ Suggested approach: use a heredoc or external file
Consider extracting the patch to a heredoc similar to the Telemetry resource below, or parsing the port from
$(JAEGER_COLLECTOR_ENDPOINT):# Alternative: Store patch in a variable or external file for readability # The port should ideally match JAEGER_COLLECTOR_ENDPOINT (currently 4318)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@make/istio.mk` at line 36, The kubectl patch command on istio.mk currently inlines a long JSON and hardcodes jaeger-collector port 4318; extract that JSON into a heredoc or separate file and reference it from the make target to improve readability, and derive the Jaeger endpoint/port from the existing JAEGER_COLLECTOR_ENDPOINT variable (or parse it into host and port) instead of hardcoding 4318 so the opentelemetry.service value uses the canonical $(JAEGER_COLLECTOR_ENDPOINT) (and keep $(TOOLS_NAMESPACE) for the service FQDN); update the istio patch invocation (the kubectl patch line) to read the payload from the heredoc/file and replace the inline JSON with a reference to that payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`:
- Around line 19-28: The fixtures (e.g., authconfig_trace, limitador_trace,
authconfig_trace_multiple_policies, limitador_trace_multiple_policies) sometimes
call skip_or_fail(...) which raises but pylint can't see that, causing
inconsistent-return-statements; after each skip_or_fail(...) call add an
explicit "return None" so every control path returns a value and the pylint
warning is silenced.
- Around line 246-249: The file ends without a trailing newline which breaks the
CI formatter; add a final newline to the end of the file (after the assert
block) so the file ends with a newline character, then run the formatter (make
reformat or poetry run black) to ensure consistent styling; locate the assert
that references found, first_policy_ref, second_policy_ref and
spans_with_sources and append the missing newline to that file.
In `@testsuite/tracing/jaeger.py`:
- Around line 62-63: The params dict in testsuite/tracing/jaeger.py expects
string values but start_time is an int, so before assigning to params["start"]
convert start_time to a string using Python's str() and then assign; update the
assignment near the conditional that checks start_time to set params["start"] to
the stringified start_time.
In `@testsuite/tracing/tempo.py`:
- Around line 30-31: The params dict in testsuite/tracing/tempo.py is getting an
int start_time assigned (params["start"] = start_time) which causes a type
mismatch; convert start_time to a string before assigning (e.g.,
str(start_time)) so params holds string values like in JaegerClient, updating
the assignment where params["start"] is set and ensuring any related uses expect
a string.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 94-101: The documentation comments include hard-coded test counts
("40 tests" and "10 tests") that can become stale; remove the numeric counts
from the two comment lines above the make commands (the lines commenting
"Control plane tracing tests (40 tests)" and "Data plane tracing tests (10
tests)") so they read generically as "Control plane tracing tests" and "Data
plane tracing tests" next to the make commands `make
testsuite/tests/singlecluster/tracing/control_plane/` and `make
testsuite/tests/singlecluster/tracing/data_plane_tracing/`.
In `@make/istio.mk`:
- Line 36: The kubectl patch command on istio.mk currently inlines a long JSON
and hardcodes jaeger-collector port 4318; extract that JSON into a heredoc or
separate file and reference it from the make target to improve readability, and
derive the Jaeger endpoint/port from the existing JAEGER_COLLECTOR_ENDPOINT
variable (or parse it into host and port) instead of hardcoding 4318 so the
opentelemetry.service value uses the canonical $(JAEGER_COLLECTOR_ENDPOINT) (and
keep $(TOOLS_NAMESPACE) for the service FQDN); update the istio patch invocation
(the kubectl patch line) to read the payload from the heredoc/file and replace
the inline JSON with a reference to that payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d1b4c2e-e087-4989-831e-6ab5163efd47
📒 Files selected for processing (13)
CLAUDE.mdconfig/settings.local.yaml.tplmake/istio.mkmake/kuadrant.mkmake/local-setup.mkmake/tools.mkmake/vars.mktestsuite/config/__init__.pytestsuite/tests/singlecluster/tracing/control_plane/test_control_plane_lifecycle.pytestsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.pytestsuite/tracing/__init__.pytestsuite/tracing/jaeger.pytestsuite/tracing/tempo.py
Comment on lines
+19
to
+28
| @pytest.fixture(scope="module") | ||
| def authconfig_trace(auth_traces, skip_or_fail): | ||
| """Find trace with authconfig span that has sources attribute""" | ||
| for trace in auth_traces: | ||
| spans = trace.filter_spans(lambda s: s.operation_name == "authconfig" and s.has_tag("sources")) | ||
| if spans: | ||
| return trace | ||
|
|
||
| skip_or_fail("No trace with authconfig span containing 'sources' attribute found") | ||
|
|
There was a problem hiding this comment.
Add explicit return to satisfy pylint's inconsistent-return-statements check.
skip_or_fail() raises an exception and never returns, but pylint doesn't know this statically. Add an explicit return None after the call to silence the warning. The same pattern applies to the other fixtures flagged by pylint (lines 31, 124, 198).
Proposed fix for this fixture
`@pytest.fixture`(scope="module")
def authconfig_trace(auth_traces, skip_or_fail):
"""Find trace with authconfig span that has sources attribute"""
for trace in auth_traces:
spans = trace.filter_spans(lambda s: s.operation_name == "authconfig" and s.has_tag("sources"))
if spans:
return trace
skip_or_fail("No trace with authconfig span containing 'sources' attribute found")
+ return None # Unreachable, but satisfies pylintApply the same pattern to limitador_trace, authconfig_trace_multiple_policies, and limitador_trace_multiple_policies fixtures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.fixture(scope="module") | |
| def authconfig_trace(auth_traces, skip_or_fail): | |
| """Find trace with authconfig span that has sources attribute""" | |
| for trace in auth_traces: | |
| spans = trace.filter_spans(lambda s: s.operation_name == "authconfig" and s.has_tag("sources")) | |
| if spans: | |
| return trace | |
| skip_or_fail("No trace with authconfig span containing 'sources' attribute found") | |
| `@pytest.fixture`(scope="module") | |
| def authconfig_trace(auth_traces, skip_or_fail): | |
| """Find trace with authconfig span that has sources attribute""" | |
| for trace in auth_traces: | |
| spans = trace.filter_spans(lambda s: s.operation_name == "authconfig" and s.has_tag("sources")) | |
| if spans: | |
| return trace | |
| skip_or_fail("No trace with authconfig span containing 'sources' attribute found") | |
| return None # Unreachable, but satisfies pylint |
🧰 Tools
🪛 GitHub Actions: Code static analysis
[error] 20-20: pylint R1710: Either all return statements in a function should return an expression, or none of them should (inconsistent-return-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`
around lines 19 - 28, The fixtures (e.g., authconfig_trace, limitador_trace,
authconfig_trace_multiple_policies, limitador_trace_multiple_policies) sometimes
call skip_or_fail(...) which raises but pylint can't see that, causing
inconsistent-return-statements; after each skip_or_fail(...) call add an
explicit "return None" so every control path returns a value and the pylint
warning is silenced.
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py (1)
109-110: Considerautouse=Truefor policy setup fixtures to align with test fixture standard.Both policy-creation fixtures already use finalisers correctly; adding
autouse=Truewould align them with the repository fixture pattern and reduce per-test wiring.As per coding guidelines: "Use
autouse=Trueandrequest.addfinalizer()in fixture setup to automatically commit policies before tests and ensure cleanup after tests complete".Also applies to: 183-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py` around lines 109 - 110, The fixture second_auth_policy (and other policy-creation fixtures) should be made autouse to follow repo conventions: change the `@pytest.fixture`(...) decorator to include autouse=True, ensure the setup still commits the policy during fixture setup, and keep the existing cleanup registered via request.addfinalizer so the policy is removed after each test; apply the same change to the other policy-creation fixture(s) referenced in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`:
- Around line 214-219: The test currently accepts a trace when either
first_policy_ref or second_policy_ref is present; change the conditional that
checks sources (the one using first_policy_ref and second_policy_ref) to require
both references (use logical AND instead of OR) so it only returns trace when
both first_policy_ref and second_policy_ref are in sources, and apply the same
change to the second occurrence of the same check (the other block that
currently uses OR before calling skip_or_fail).
- Around line 140-146: The test currently accepts a trace if either policy ref
appears in the trace's sources; change that to require both policies so the test
validates multiple-policy propagation: in the block that checks "if sources and
(first_policy_ref in sources or second_policy_ref in sources)" replace the
logical OR with AND (or equivalently assert both first_policy_ref and
second_policy_ref are in sources) so the function returns the trace only when
both are present; make the same change in the second occurrence that mirrors
lines ~173-180 so both checks require both refs, keeping the surrounding use of
variables first_policy_ref, second_policy_ref, sources, trace and the
skip_or_fail behavior unchanged.
- Line 73: The filter on limitador_trace currently picks any span with the
"sources" tag and may match unrelated spans; update the lambda used in
limitador_trace.filter_spans(...) so it only returns spans that both
have_tag("sources") and whose operation_name == "limits" (e.g., change lambda s:
s.has_tag("sources") to lambda s: s.has_tag("sources") and s.operation_name ==
"limits") where limitador_span is selected (also apply the same change to the
other two occurrences that assert on limitador spans).
---
Nitpick comments:
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`:
- Around line 109-110: The fixture second_auth_policy (and other policy-creation
fixtures) should be made autouse to follow repo conventions: change the
`@pytest.fixture`(...) decorator to include autouse=True, ensure the setup still
commits the policy during fixture setup, and keep the existing cleanup
registered via request.addfinalizer so the policy is removed after each test;
apply the same change to the other policy-creation fixture(s) referenced in the
review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8052dbb5-42c2-49d2-b5ff-474d0298205b
📒 Files selected for processing (1)
testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py
| """ | ||
| Validate that limitador limits reconciliation spans include sources, name, and namespace attributes. | ||
| """ | ||
| limitador_span = limitador_trace.filter_spans(lambda s: s.has_tag("sources"))[0] |
There was a problem hiding this comment.
Filter limitador assertions to operation_name == "limits" to avoid false positives.
On Line 73, Line 211, and Line 231 the code selects any span with sources, which can match unrelated spans. Constrain these to limits spans so assertions target the intended reconciliation span.
Proposed fix
- limitador_span = limitador_trace.filter_spans(lambda s: s.has_tag("sources"))[0]
+ limitador_span = limitador_trace.filter_spans(
+ lambda s: s.operation_name == "limits" and s.has_tag("sources")
+ )[0]
@@
- spans = trace.filter_spans(lambda s: s.has_tag("sources"))
+ spans = trace.filter_spans(lambda s: s.operation_name == "limits" and s.has_tag("sources"))
@@
- spans_with_sources = limitador_trace_multiple_policies.filter_spans(lambda s: s.has_tag("sources"))
+ spans_with_sources = limitador_trace_multiple_policies.filter_spans(
+ lambda s: s.operation_name == "limits" and s.has_tag("sources")
+ )Also applies to: 211-213, 231-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`
at line 73, The filter on limitador_trace currently picks any span with the
"sources" tag and may match unrelated spans; update the lambda used in
limitador_trace.filter_spans(...) so it only returns spans that both
have_tag("sources") and whose operation_name == "limits" (e.g., change lambda s:
s.has_tag("sources") to lambda s: s.has_tag("sources") and s.operation_name ==
"limits") where limitador_span is selected (also apply the same change to the
other two occurrences that assert on limitador spans).
Comment on lines
+140
to
+146
| if sources and (first_policy_ref in sources or second_policy_ref in sources): | ||
| return trace | ||
|
|
||
| return skip_or_fail( | ||
| f"No trace with authconfig span found with either policy. " | ||
| f"Looking for {first_policy_ref} or {second_policy_ref} in sources" | ||
| ) |
There was a problem hiding this comment.
Require both AuthPolicy references, not either one.
The current logic passes when only one policy appears, so it does not verify the “multiple policies” objective. On Line 140 and Line 173, switch or to and (or assert both refs in the same sources list) so this test only passes when both policies are represented.
Proposed fix
- if sources and (first_policy_ref in sources or second_policy_ref in sources):
+ if sources and (first_policy_ref in sources and second_policy_ref in sources):
return trace
@@
- if first_policy_ref in sources or second_policy_ref in sources:
+ if first_policy_ref in sources and second_policy_ref in sources:
found = True
break
@@
- f"Neither {first_policy_ref} nor {second_policy_ref} found in any authconfig span sources. "
+ f"Both {first_policy_ref} and {second_policy_ref} must be present in authconfig span sources. "
f"Checked {len(authconfig_spans)} span(s)"
)Also applies to: 173-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`
around lines 140 - 146, The test currently accepts a trace if either policy ref
appears in the trace's sources; change that to require both policies so the test
validates multiple-policy propagation: in the block that checks "if sources and
(first_policy_ref in sources or second_policy_ref in sources)" replace the
logical OR with AND (or equivalently assert both first_policy_ref and
second_policy_ref are in sources) so the function returns the trace only when
both are present; make the same change in the second occurrence that mirrors
lines ~173-180 so both checks require both refs, keeping the surrounding use of
variables first_policy_ref, second_policy_ref, sources, trace and the
skip_or_fail behavior unchanged.
Comment on lines
+214
to
+219
| if sources and (first_policy_ref in sources or second_policy_ref in sources): | ||
| return trace | ||
|
|
||
| return skip_or_fail( | ||
| f"No trace found with either policy. Looking for {first_policy_ref} or {second_policy_ref} in sources" | ||
| ) |
There was a problem hiding this comment.
Require both RateLimitPolicy references in multi-policy validation.
Like the authconfig variant, this currently succeeds when only one policy is present. On Line 214 and Line 244, enforce both references in sources so the test actually validates aggregation.
Proposed fix
- if sources and (first_policy_ref in sources or second_policy_ref in sources):
+ if sources and (first_policy_ref in sources and second_policy_ref in sources):
return trace
@@
- if first_policy_ref in sources or second_policy_ref in sources:
+ if first_policy_ref in sources and second_policy_ref in sources:
found = True
break
@@
- f"Neither {first_policy_ref} nor {second_policy_ref} found in any span sources. "
+ f"Both {first_policy_ref} and {second_policy_ref} must be present in span sources. "
f"Checked {len(spans_with_sources)} span(s)"
)Also applies to: 244-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@testsuite/tests/singlecluster/tracing/control_plane/test_control_plane_sources.py`
around lines 214 - 219, The test currently accepts a trace when either
first_policy_ref or second_policy_ref is present; change the conditional that
checks sources (the one using first_policy_ref and second_policy_ref) to require
both references (use logical AND instead of OR) so it only returns trace when
both first_policy_ref and second_policy_ref are in sources, and apply the same
change to the second occurrence of the same check (the other block that
currently uses OR before calling skip_or_fail).
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
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
1 participant
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.
Description
Implements issue #919 by adding comprehensive test coverage for control plane tracing
sourcesattributes that link policy reconciliation spans to the actual policies that triggered them. Additionally includes improvements to tracing infrastructure to handle fast reconciliation environments like kind.sourcesattributes in authconfig and limitador spansChanges
New Features
test_control_plane_sources.py: New test file with 5 comprehensive tests for sources attributestest_authconfig_span_attributes: Validates authconfig spans have sources, name, and namespace attributes with correct AuthPolicy referencestest_limitador_span_attributes: Validates limitador spans have sources, name, and namespace attributes with correct RateLimitPolicy referencestest_authconfig_span_is_child_of_reconciler: Validates authconfig spans are children ofreconciler.auth_configsspanstest_authconfig_sources_contains_multiple_policies: Tests multiple AuthPolicies targeting same route appear in sourcestest_limitador_sources_contains_multiple_policies: Tests multiple RateLimitPolicies targeting same route appear in sourcesBug Fixes
start_timeparameter toJaegerClient.get_traces(),RemoteTempoClient.get_traces(), andTracingClientbase class to support timestamp-based trace queryingtest_control_plane_lifecycle.py: Fixed race conditions intest_policy_update_generates_new_reconciliation_traceandtest_policy_target_change_tracedby capturing precise timestamps and usingstart_timeinstead of relying on snapshot comparison alonemake/vars.mkandtestsuite/config/__init__.pyrpc://tohttp://for OTLP/gRPC endpointsInfrastructure
make/local-setup.mk,make/tools.mk,make/istio.mk,make/kuadrant.mkCLAUDE.mdwith comprehensive Jaeger tracing access instructions including control plane and data plane tracing setupVerification steps
Run the new tests:
To test locally with Jaeger tracing:
make local-setup kubectl port-forward -n tools svc/jaeger-query 16686:80 # Open http://localhost:16686 to view tracesRelated Issues
Closes #919
Summary by CodeRabbit
New Features
Documentation
Tests
Chores