-
Notifications
You must be signed in to change notification settings - Fork 631
Add AgentgatewayPolicy frontend tracing support #13226
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.
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
Add AgentgatewayPolicy frontend tracing support #13226
Conversation
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: npolshakova <[email protected]>
3cb7014 to
cd5bd31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds frontend tracing support for AgentgatewayPolicy, enabling OpenTelemetry tracing configuration for incoming gateway traffic. The implementation includes new API types for resource attributes, translation logic to convert policies to agent gateway format, and updated CRD schemas with validation rules.
Key changes:
- Added
Resourcesfield to the Tracing spec for configuring OTEL resource attributes - Implemented translation logic in
translateFrontendTracingto convert tracing policies to agent gateway API format - Removed the CRD validation that blocked tracing configuration (
x-kubernetes-validationsrule)
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/agentgateway/agentgateway_policy_types.go |
Added ResourceAdd type and Resources field to Tracing struct; removed "not implemented" comments |
api/v1alpha1/agentgateway/zz_generated.deepcopy.go |
Auto-generated deepcopy methods for new ResourceAdd type and updated Tracing deepcopy to handle Resources slice |
install/helm/agentgateway-crds/templates/agentgateway.dev_agentgatewaypolicies.yaml |
Added resources field schema to tracing spec; removed validation blocking tracing implementation; fixed grammar in attributes description |
pkg/agentgateway/plugins/frontend_policies.go |
Implemented translateFrontendTracing function to convert tracing policy to agent gateway format, including backend reference, attributes, resources, and sampling configuration |
pkg/agentgateway/plugins/testdata/frontendpolicy/full.yaml |
Added complete tracing test case with expected output showing proper translation of all tracing fields |
test/e2e/features/tracing/testdata/setup.yaml |
Removed GatewayParameters, Gateway, and HTTPRoute definitions (likely moved to separate e2e PR mentioned in description) |
go.mod |
Added temporary local replace directive for agentgateway dependency |
go.sum |
Removed agentgateway v0.11.0 checksums due to local replacement |
hack/utils/oss_compliance/osa_provided.md |
Updated agentgateway dependency version from v0.11.0 to "latest" |
Comments suppressed due to low confidence (1)
go.mod:672
- This is a temporary local replacement for the agentgateway dependency that should be removed before merging. The PR description mentions that an agentgateway release is required before this can be merged. Ensure this replace directive is removed and the dependency is updated to the proper version when that release is available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: npolshakova <[email protected]>
Signed-off-by: Lawrence Gadban <[email protected]>
|
|
||
| var randomSampling *string | ||
| if tracing.RandomSampling != nil { | ||
| randomSampling = ptr.Of(string(*tracing.RandomSampling)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
castPtr helps hgere
|
|
||
| provider, err := buildBackendRef(ctx, tracing.BackendRef, policy.Namespace) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to translate tracing backend ref: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed in this PR but this is one of the broader things we discussed recently -- we should be sending tracing config with an invalid backend probably. But we can fix it more holistically and not in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use #13231 as a starting point for this convo
| } | ||
|
|
||
| func translateFrontendTracing(policy *agentgateway.AgentgatewayPolicy, name string, target *api.PolicyTarget) []AgwPolicy { | ||
| func translateFrontendTracing(ctx PolicyCtx, policy *agentgateway.AgentgatewayPolicy, name string, target *api.PolicyTarget) ([]AgwPolicy, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the convention is to name the PolicyCtx variable/param as ctx.
as a new reader this is a bit confusing given how overloaded ctx is with Context
|
@howardjohn can you reapprove? I had to fix one of the e2e tests and for some reason it thinks your approval came before that commit... |
NOTE: Requires agentgateway release before this can be merged.
Description
Fixes: #11818
Change Type
Changelog
Additional Notes
e2e tests are introduced in a separate follow up PR: #13076