feat(kuma-cp) Generate tracing config in Bootstrap config #592
Conversation
This file contains 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
jakubdyszkiewicz
force-pushed
the
feature/generate-tracing-on-listeners
branch
from
ed8d010 to
cd4c738
Compare
jakubdyszkiewicz
force-pushed
the
feature/tracing-bootstrap
branch
from
96c2eff to
11fdf80
Compare
yskopets
approved these changes
Feb 20, 2020
| if zipkin.ApiVersion != "" && zipkin.ApiVersion != "httpJson" && zipkin.ApiVersion != "httpProto" { | ||
| verr.AddViolation("apiVersion", `has to be either "httpJson" or "httpProto"`) | ||
| if zipkin.ApiVersion != "" && zipkin.ApiVersion != "httpJsonV1" && zipkin.ApiVersion != "httpJson" && zipkin.ApiVersion != "httpProto" { | ||
| verr.AddViolation("apiVersion", `has to be one of the following values: "httpJsonV1", "httpJson" or "httpProto"`) |
There was a problem hiding this comment.
Could we reuse AllowedValuesHint() ?
| if zipkin.ApiVersion != "" && zipkin.ApiVersion != "httpJson" && zipkin.ApiVersion != "httpProto" { | ||
| verr.AddViolation("apiVersion", `has to be either "httpJson" or "httpProto"`) | ||
| if zipkin.ApiVersion != "" && zipkin.ApiVersion != "httpJsonV1" && zipkin.ApiVersion != "httpJson" && zipkin.ApiVersion != "httpProto" { | ||
| verr.AddViolation("apiVersion", `has to be one of the following values: "httpJsonV1", "httpJson" or "httpProto"`) |
There was a problem hiding this comment.
Can we update docs for Zipkin proto type to mention httpJsonV1 value as well ?
And probably describe the difference or reference Envoy docs ?
| const zipkinClusterTimeout = 10 * time.Second | ||
|
|
||
| func zipkinCluster(backendName string, url *net_url.URL) (*envoy_api.Cluster, error) { | ||
| port, err := strconv.Atoi(url.Port()) |
There was a problem hiding this comment.
This will result in the error if URL has no port.
If the port is absolutely mandatory, should we update Mesh validator then ?
jakubdyszkiewicz
force-pushed
the
feature/generate-tracing-on-listeners
branch
2 times, most recently
from
ae11c40 to
910a126
Compare
jakubdyszkiewicz
force-pushed
the
feature/tracing-bootstrap
branch
from
11fdf80 to
2ed5a5c
Compare
jakubdyszkiewicz
changed the base branch from
feature/generate-tracing-on-listeners
to
master
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Generate tracing config in Bootstrap config.
When we generate snapshot, we do traffic trace matching and get proper backend from the mesh. Then we convert this to bootstrap config.
Note: There is some bug in Envoy - Zipkin V2 version of the API is not compatible with Jaeger, because timestamp field in JSON is string and Jeager expects int64 there. We can look for it as a separate task and just use V1 for now which works fine.