feat: trace watchable messages #7403
Conversation
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
| } | ||
|
|
||
| // EnvoyGatewayTraces defines control plane tracing configurations. | ||
| type EnvoyGatewayTraces struct { |
There was a problem hiding this comment.
In EG, we use tracing in other place, better to keep consistency.
gateway/api/v1alpha1/envoygateway_types.go
Line 524 in 4c36666
There was a problem hiding this comment.
I didn't understand your point.
There was a problem hiding this comment.
the name should be EnvoyGatewayTracing instead of EnvoyGatewayTraces.
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (46.26%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7403 +/- ##
==========================================
- Coverage 72.26% 71.18% -1.09%
==========================================
Files 231 275 +44
Lines 34084 35145 +1061
==========================================
+ Hits 24632 25019 +387
- Misses 7676 8322 +646
- Partials 1776 1804 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
thanks for working on this @shreealt is it possible to break this feature up into 2
|
internal/xds/runner/runner.go
Outdated
| parentCtx = update.Value.Context | ||
| } | ||
|
|
||
| _, span := tracer.Start(parentCtx, "XdsRunner.subscribeAndTranslate") |
There was a problem hiding this comment.
can we add another child span to r.cache.GenerateNewSnapshot and use the trace id as the version inside it, so this id trickles into envoy proxy
There was a problem hiding this comment.
done, but I hope this doesn't come off as a surprise to users?
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
hey @shreealt left some comments, once those are addressed, can you also share before/after logs, tia ! |
|
@shreealt we'll also need |
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
/retest |
Signed-off-by: Shreemaan Abhishek <[email protected]>
| emptyClusterName = "EmptyCluster" | ||
| ) | ||
|
|
||
| var tracer = otel.Tracer("envoy-gateway/xds/translator") |
There was a problem hiding this comment.
lets not add any Otel things to the lib, but in the runner please, in case these libs are made public and used purely for translation
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
shreealt
force-pushed
the
feat/tracing
branch
from
ab45164 to
cd7fdd6
Compare
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
hey @shreealt can you add some relevant before/after logs to the PR |
done. |
|
/retest |
internal/gatewayapi/runner/runner.go
Outdated
| var backendTLSPolicyStatusCount, clientTrafficPolicyStatusCount, backendTrafficPolicyStatusCount int | ||
| var securityPolicyStatusCount, envoyExtensionPolicyStatusCount, backendStatusCount, extensionServerPolicyStatusCount int | ||
|
|
||
| span.AddEvent("gateway_resources_translation_cycle", trace.WithAttributes(attribute.Int("resources.count", len(*val)))) |
There was a problem hiding this comment.
thoughts on just translate ? , since tracer already has gateway-api runner info
There was a problem hiding this comment.
sorry I did not get you, did you intend to name this event just translate?
internal/xds/runner/runner.go
Outdated
|
|
||
| // Add span attributes for observability | ||
| span.SetAttributes( | ||
| attribute.String("controller.key", update.Key), |
internal/logging/log.go
Outdated
| fields := []interface{}{ | ||
| "trace_id", sc.TraceID().String(), | ||
| "span_id", sc.SpanID().String(), | ||
| "trace_flags", sc.TraceFlags().String(), |
There was a problem hiding this comment.
imo not needed for logging purposes
| defer span.End() | ||
|
|
||
| span.SetAttributes( | ||
| attribute.String("controller.key", update.Key), |
internal/cmd/server.go
Outdated
| runner: metrics.New(cfg), | ||
| }, | ||
| { | ||
| // Start the Traces Server |
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
/retest |
|
tests are failing |
Signed-off-by: Shreemaan Abhishek <[email protected]>
|
whoops, fixed it sorry. |
| } else { | ||
| // Translate to ratelimit xDS Config. | ||
| rvt, err := r.translate(update.Value) | ||
| rvt, err := r.translate(update.Value.XdsIR) |
There was a problem hiding this comment.
can we also add a span to this
| return ptr.To(false) | ||
| } | ||
|
|
||
| var tracer = otel.Tracer("envoy-gateway/reconciliation") |
There was a problem hiding this comment.
| var tracer = otel.Tracer("envoy-gateway/reconciliation") | |
| var tracer = otel.Tracer("envoy-gateway/provider/runner") |
|
hey @shreealt, added a few more minor comments, but this is close to being merged. since this PR touches the watchable messages, can you run a test locally to prove that pushing identical messages dont cause churn (watchable subscribes)
|
3 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #7324
Release Notes: Yes/No
Logs
Before:
After: