-
Notifications
You must be signed in to change notification settings - Fork 327
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
Dynamic tracing #930
Dynamic tracing #930
Conversation
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
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.
Couple minor comments, nothing serious. Overall looks great, I like the direction we've taken with e2e!
@@ -31,3 +31,7 @@ func GetEnvoyAdminClusterName() string { | |||
func GetPrometheusListenerName() string { | |||
return "kuma:metrics:prometheus" | |||
} | |||
|
|||
func GetTracingCluster(backendName string) string { |
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.
GetTracingClusterName
in order to be consistent with the whole other file
switch c := cluster.(type) { | ||
case *framework.K8sCluster: | ||
deployment := &k8SDeployment{} | ||
c.Deployments[DeploymentName] = deployment |
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.
I don't really like these direct writes to the map. Can we extend framework.Deployment
interface with a new method Name() string
and add a new method to framework.Cluster
interfaces:
func (c *K8sCluster) Deploy(d Deployment) error {
c.deployment[d.Name()] = d
return d.Deploy(c)
}
envoy_endpoints "github.com/kumahq/kuma/pkg/xds/envoy/endpoints" | ||
) | ||
|
||
func DNSCluster(name string, address string, port uint32) ClusterBuilderOpt { |
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.
Is that only for the Dynamic tracing, or it will allow to use DNS names in other filters too?
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.
Maybe in the future, but for now - not really. There are several cluster types:
- EDS cluster - most commonly used, it relies on ClusterLoadAssignment on endpoints which cannot be DNS names.
- Static cluster - it requires static endpoints that are IPs. It is used only for inbound cluster (which IP we know from DP definition and it should be IP because it is then used in outbounds of other DPs) and Prometheus (admin IP)
- original dst - transparent proxy
- strict DNS - used here since an address can be a DNS name.
@@ -45,7 +47,7 @@ spec: | |||
- -e | |||
- echo | |||
- -e | |||
- "HTTP/1.1 200 OK" | |||
- "HTTP/1.1 200 OK\n\n" |
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.
👍
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Summary
Kuma now leverages tracing configuration on HTTP Connection Manager introduced in Envoy 1.15.
Testing
I added E2E test to check if traces are sent to Jeager on Universal and Kubernetes.
To avoid polluting
Cluster
interface with even more methods I introduced the concept ofDeployment
.Deployment
is work unit that can be deployed onCluster
and it exposes user friendly interface for testing. So for example for tracing it'sTracedService()
andZipkinCollectorURL()
. I believe that Kuma CP, Kuma DP, demo app, demo client should all be deployments, so for example demo client it could beso you don't have to look for pods/containers and construct curls.
Tests should be as simple as possible to read.
Documentation