OpenTelemetryTracingProvider: New configurations for resource detectors

[joaopgrassi]

This PR is an alternative to #2998 to showcase "all" new MeshConfig properties we would need to have to support all the new OpenTelemetry tracing features in Envoy. I'm creating this so the Istio community can see it and maybe this influences the discussions.

• HTTP Exporter
• Custom Sampler
• Resource Detectors

All of those features are defined by the OpenTelemetry project and were added to Envoy as a mechanism to enhance the traces produced by it. More specially, because with the eminent OpenTracing deprecation there, some observability back-ends will be affected and by improving the OTel support in Envoy users can switch to that.

EDIT

I modified this PR to only include the resource detectors. Sampling will be added in a separated PR to avoid bloating.

Closes istio/istio#48885

[istio-testing]

Hi @joaopgrassi. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[joaopgrassi]

Note: I have a smaller PR that just introduces the HTTP exporter #2998. This was just to show the "full" amount of new configuration we will need to support the new OTel tracing features in Envoy

[zirain]

/ok-to-test

[zirain]

#2998 merged, can we close this? cc @joaopgrassi

[joaopgrassi]

@zirain #2998 added only the HTTP exporter configuration. We still need new configuration for the resource detectors part and samplers. I will rebase this and remove the http exporter, as that is already in, but the rest here still apply.

[costinm]

Can you add a small comment with the OTLP env variables that set the equivalent percentage ? Just for reference.

…

On Tue, Jan 30, 2024 at 11:30 AM John Howard ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mesh/v1alpha1/config.proto <#3002 (comment)>: > @@ -1025,6 +1026,53 @@ message MeshConfig { // Optional. Controls the overall path length allowed in a reported span. // NOTE: currently only controls max length of the path tag. uint32 max_tag_length = 3; + + // Optional. Specifies the configuration for exporting OTLP traces via HTTP. + // When empty, traces will be exported via gRPC. + HttpService otlp_http = 4; nit: can you rebase? ------------------------------ In mesh/v1alpha1/config.proto <#3002 (comment)>: > @@ -1025,6 +1026,53 @@ message MeshConfig { // Optional. Controls the overall path length allowed in a reported span. // NOTE: currently only controls max length of the path tag. uint32 max_tag_length = 3; + + // Optional. Specifies the configuration for exporting OTLP traces via HTTP. + // When empty, traces will be exported via gRPC. + HttpService otlp_http = 4; + + // Optional. Specifies a list of resource detectors to be used by the OpenTelemetry Tracer. + repeated google.protobuf.Any resource_detectors = 5; I guess I don't understand why for one we use concrete types and the other Any. Seems like we should be consistent in both area, unless there is a compelling reason to do them differently? ------------------------------ In mesh/v1alpha1/config.proto <#3002 (comment)>: > @@ -1025,6 +1026,53 @@ message MeshConfig { // Optional. Controls the overall path length allowed in a reported span. // NOTE: currently only controls max length of the path tag. uint32 max_tag_length = 3; + + // Optional. Specifies the configuration for exporting OTLP traces via HTTP. + // When empty, traces will be exported via gRPC. + HttpService otlp_http = 4; + + // Optional. Specifies a list of resource detectors to be used by the OpenTelemetry Tracer. + repeated google.protobuf.Any resource_detectors = 5; + + // Optional. Specifies the sampler to be used by the OpenTelemetry Tracer + // When specified, it overrides the sampler configuration from Envoy. + oneof sampler { + AlwaysOnSampler always_on = 6; Should we: 1. Remove AlwaysOn; if users want this they should set randomSamplingPercentage=100%. no need to have many ways to say "always on" 2. Document the behavior of these interaction with other sampling percentage options (i.e. percentage is ignored/set to 100%) 3 — Reply to this email directly, view it on GitHub <#3002 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2W2O76JQFRD5F3P62TYRFC4XAVCNFSM6AAAAAA73ZPA5OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJSGE3TANBUG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[joaopgrassi]

Hi all, I have rebased this PR on latest master, given #2998 was merged. I think we have some common understanding/agreement on how to go forward so I also removed the sampling parts of the PR. I will focus this on resource detectors and send a new PR for sampling once we have the code merged in Envoy for the Dynatrace sampler.

CC @howardjohn @zirain

[zirain]

LGTM, thanks!

[linsun]

Per @zirain's req, we discussed the request to cherry pick this to Istio 1.21 branch in today's istio WG meeting. The general consensus is all api changes will require TOC approval to cherry pick if this misses the release branch cutting. Given there is still impl PR open on this and we don't see a strong reason to make an exception for cherry pick this API change, we prefer to not cherry pick and wait till it lands in istio 1.22.

[linsun]

cc @istio/release-managers-1-21 ^^^