Skip to content
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

feat(MeshTrace): add support for opentelemetry trace backend #5992

Merged
merged 4 commits into from
Feb 13, 2023

Conversation

frzifus
Copy link
Contributor

@frzifus frzifus commented Feb 9, 2023

This pr adds support to export traces in otel format. it is still in an early and experimental stage. ^^

Closes #3690

Checklist prior to review

  • Link to docs PR or issue -- OpenTelemetry support for tracing #3690
  • Link to UI issue or PR -- None
  • Is the issue worked on linked? --
  • The PR does not hardcode values that might break projects that depend on kuma (e.g. "kumahq" as a image registry) --
  • The PR will work for both Linux and Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Unit Tests --
  • E2E Tests --
  • Manual Universal Tests --
  • Manual Kubernetes Tests --
  • Do you need to update UPGRADE.md? -- No
  • Does it need to be backported according to the backporting policy? -- No
  • Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

See: envoyproxy/envoy#9958 (comment)

@mmorel-35
Copy link
Contributor

Hi @frzifus ,
Have you considered this reference to define mesh_proto.OTLPTracingBackendConfig

@frzifus
Copy link
Contributor Author

frzifus commented Feb 10, 2023

Have you considered this reference to define mesh_proto.OTLPTracingBackendConfig

What are the settings that come to your mind / you did like to see? Until here I just went for a simple test. ^^

@mmorel-35
Copy link
Contributor

mmorel-35 commented Feb 10, 2023

What are the settings that come to your mind / you did like to see?

I’d like to see this for example :

conf:
  compression: gzip
  endpoint: otelcol2:55690
  headers:
    test1: "value1"
  keepalive:
    timeout: 20s
  tls:
    insecure: false
    ca_file: ca.pem
    cert_file: cert.pem
    key_file: key.pem
    insecure_skip_verify: false
    min_version: "1.2"
    max_version: "1.3"

@frzifus frzifus force-pushed the add_otel_trace_support branch from a83f5ef to 5184ce7 Compare February 11, 2023 21:02
@frzifus frzifus changed the title poc: add otel trace support feat(policy): add otel trace support Feb 11, 2023
@frzifus frzifus force-pushed the add_otel_trace_support branch from 5184ce7 to 67d45cb Compare February 11, 2023 21:03
@frzifus
Copy link
Contributor Author

frzifus commented Feb 11, 2023

I’d like to see this for example :

But these are the internal opentelemetry grpc client settings. If I got it right, we can only use the envoy gRPC settings.

@frzifus frzifus force-pushed the add_otel_trace_support branch 2 times, most recently from 2908bbc to 1af4dc2 Compare February 11, 2023 21:47
@frzifus frzifus changed the title feat(policy): add otel trace support feat(policy): add opentelemetry trace backend support Feb 11, 2023
@frzifus frzifus changed the title feat(policy): add opentelemetry trace backend support feat(policy): add support for opentelemetry trace backend Feb 11, 2023
@frzifus frzifus force-pushed the add_otel_trace_support branch from 1af4dc2 to c09991d Compare February 11, 2023 22:55
@mmorel-35
Copy link
Contributor

That is the opentelemetry exporter grpc client specification, right. This is the protobuf part. Then it needs to be translated into envoy configuration, if I understood it right

@frzifus
Copy link
Contributor Author

frzifus commented Feb 11, 2023

This is not a specification. It is simply a grpc client implementation that is used in the opentelemetry collector {exporter, extensions, ...}. Only these settings are available to us.

@frzifus frzifus marked this pull request as ready for review February 12, 2023 00:01
@frzifus frzifus requested review from a team, slonka and bartsmykla and removed request for a team February 12, 2023 00:01
@frzifus
Copy link
Contributor Author

frzifus commented Feb 12, 2023

hm.. while working on an e2e test, I noticed that the Jaeger deployment provided by kumactl does not expose otlp.
Should we put this into this pr too. Or do it in a folllow-up? Iam a bit scared that its gets bigger and bigger until it leads to conflicts. ^^
wdyt?

@frzifus frzifus force-pushed the add_otel_trace_support branch 2 times, most recently from 14af4c5 to 5ccee44 Compare February 12, 2023 11:38
@frzifus frzifus changed the title feat(policy): add support for opentelemetry trace backend feat(policy,kumactl): add support for opentelemetry trace backend Feb 12, 2023
@michaelbeaumont michaelbeaumont changed the title feat(policy,kumactl): add support for opentelemetry trace backend feat(MeshTrace): add support for opentelemetry trace backend Feb 12, 2023
@frzifus frzifus force-pushed the add_otel_trace_support branch from 5ccee44 to 60db9a0 Compare February 12, 2023 12:12
@frzifus frzifus removed the request for review from slonka February 12, 2023 12:28
@frzifus frzifus force-pushed the add_otel_trace_support branch 3 times, most recently from 48ccfa8 to b2fd267 Compare February 12, 2023 16:49
@frzifus frzifus force-pushed the add_otel_trace_support branch from b2fd267 to ddcd50b Compare February 12, 2023 17:05
@frzifus frzifus force-pushed the add_otel_trace_support branch from ddcd50b to 60ab66e Compare February 12, 2023 19:35
@frzifus frzifus force-pushed the add_otel_trace_support branch from 60ab66e to 9fbb480 Compare February 12, 2023 19:44
@mmorel-35
Copy link
Contributor

mmorel-35 commented Feb 12, 2023

I have found this documentation from Opentelemetry . Have you seen it ?
This part seems to be absent in the actual configuration, shall it be added too ?

      lb_policy: ROUND_ROBIN
      typed_extension_protocol_options:
        envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
          "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
          explicit_http_config:
            http2_protocol_options: {}

Also , there is no timeout defined for the grpc_service

@frzifus
Copy link
Contributor Author

frzifus commented Feb 12, 2023

I have found this documentation from Opentelemetry . Have you seen it ?
This part seems to be absent in the actual configuration, shall it be added too ?

Yes, while adding the endpoint. But tbh I would prefer to continue and add things afterwards step by step.

@mmorel-35
Copy link
Contributor

I would prefer to continue and add things afterwards step by step.

Ok ! That’s alright for me :) !

@frzifus frzifus force-pushed the add_otel_trace_support branch from 9fbb480 to 2823ded Compare February 12, 2023 22:00
@frzifus frzifus force-pushed the add_otel_trace_support branch from 2823ded to a441ab3 Compare February 13, 2023 11:34
pkg/plugins/policies/meshtrace/api/v1alpha1/validator.go Outdated Show resolved Hide resolved
pkg/plugins/policies/meshtrace/api/v1alpha1/validator.go Outdated Show resolved Hide resolved
type OpenTelemetryBackend struct {
// Address of OpenTelemetry collector.
// +kubebuilder:example="otel-collector:4317"
Endpoint string `json:"endpoint"`
Copy link
Contributor

@michaelbeaumont michaelbeaumont Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Endpoint string `json:"endpoint"`
// +kubebuilder:validation:MinLength=1
Endpoint string `json:"endpoint"`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelbeaumont - is there any reason it's not called Url to be consistent with other backends? (zipkin, datadog)

Copy link
Contributor

@mmorel-35 mmorel-35 Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be due to the absence of protocol in that field, which is defined for Zipkin and Datadog?

pkg/plugins/policies/meshtrace/plugin/v1alpha1/plugin.go Outdated Show resolved Hide resolved
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
- bump jaeger from 1.34.1 to 1.42.0

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus frzifus force-pushed the add_otel_trace_support branch from a441ab3 to 67fda65 Compare February 13, 2023 15:54
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@michaelbeaumont michaelbeaumont merged commit e84dc74 into kumahq:master Feb 13, 2023
@frzifus frzifus deleted the add_otel_trace_support branch February 13, 2023 16:47
@frzifus
Copy link
Contributor Author

frzifus commented Feb 13, 2023

Thanks for guidance @michaelbeaumont !

type OpenTelemetryBackend struct {
// Address of OpenTelemetry collector.
// +kubebuilder:example="otel-collector:4317"
Endpoint string `json:"endpoint"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelbeaumont - is there any reason it's not called Url to be consistent with other backends? (zipkin, datadog)

@@ -200,6 +205,19 @@ func endpointForZipkin(cfg *api.ZipkinBackend) *xds.Endpoint {
}
}

func endpointForOpenTelemetry(cfg *api.OpenTelemetryBackend) *xds.Endpoint {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned in the previous comment if it was a url then we could simply do:

url.Port()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTelemetry support for tracing
4 participants