opentelemetrytracer: Dynatrace sampler: Use http_service in configura… #33034

samohte · 2024-03-21T14:56:33Z

Commit Message: change Dynatrace sampler config to use http_service
Additional Description: In #32598 a custom Dynatrace sampler was added to Envoy. This sampler fetches its configuration via an HTTP call from a Dynatrace cluster. The endpoint and some additional information required to perform the HTTP call can be set in the Envoy config file.
While working on Istio changes to allow to set the endpoint, it turned out that it would be easier to use http_service instead of http_uri.
This also allows to configure additional HTTP headers which are added to the request.
Another benefit is that the configuration is now aligned to the HTTP exporter, see https://github.com/envoyproxy/envoy/blob/main/api/envoy/config/trace/v3/opentelemetry.proto#L42

Since the Dynatrace sampler is not yet released it should be ok to do config breaking change.

Risk Level: LOW
Testing: Unit, Integration, Manual
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #Issue] N/A
[Optional Fixes commit #PR or SHA] #32598, #32848
[Optional Deprecated:] N/A
[Optional API Considerations:] N/A

…tion. (#22) * change dynatrace sampler config to use http_service Signed-off-by: thomas.ebner <[email protected]> Signed-off-by: Thomas Ebner <[email protected]> Co-authored-by: Joao Grassi <[email protected]>

repokitteh-read-only · 2024-03-21T14:56:37Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #33034 was opened by samohte.

see: more, trace.

repokitteh-read-only · 2024-03-21T14:56:42Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33034 was opened by samohte.

see: more, trace.

…ce-to-configure-dynatrace-sampler Signed-off-by: thomas.ebner <[email protected]>

Signed-off-by: thomas.ebner <[email protected]>

wbpcode · 2024-03-22T09:08:53Z

#32598 was merged at three weeks ago. So you cannot change the proto API in that way directly.

/wait

joaopgrassi · 2024-03-22T09:25:02Z

@wbpcode oh really? Even though this isn't released yet? Is there anything we should do, like force the change or smt?

wbpcode · 2024-03-22T13:20:34Z

@envoyproxy/api-shepherds

wbpcode · 2024-03-22T13:24:38Z

Changes made within 14 days of the introduction of a new API field or message, provided the new field or message has not been included in an Envoy release.

I think you have missed the time window.

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto

field. Signed-off-by: thomas.ebner <[email protected]>

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto

source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc

htuch · 2024-03-26T04:22:06Z

What does "Since the Dynatrace sampler is not yet released it should be ok to do config breaking change." mean here? Generally we operate on the principle that HEAD is always a release candidate and shoudl abide by principle described by @wbpcode.

That said, if for some reason there is 0% chance someone might be sensibly ysing Dynatrace (e.g. the implementation was not fully completed and this is a a brand new extension), maybe there is an ability to introduce an exception here. As soon as someone can reasonably be using this feature and the 14 day window expires, we need to follow proper API stability protocol though.

joaopgrassi · 2024-03-26T15:39:19Z

What does "Since the Dynatrace sampler is not yet released it should be ok to do config breaking change." mean here? Generally we operate on the principle that HEAD is always a release candidate and shoudl abide by principle described by

@htuch yeah unfortunately we weren't aware that HEAD is a release candidate and about the 14 day window for changes so we thought since it isn't released yet it would be OK to make such changes.

That said, if for some reason there is 0% chance someone might be sensibly ysing Dynatrace (e.g. the implementation was not fully completed and this is a a brand new extension),

@htuch from the Dynatrace side, we can be certain that this is not used. We are not finished in the Dynatrace cluster side to make this work (configuration endpoint that this extension requires and polls from is only going to be GA in some weeks). Also, the work to enable and use this in Istio is also not merged istio/api#3134 and we never announced/promoted this to any customer of ours. Also yes, this is a new extension that we added exactly a month ago #32598

We have done the code changes to still keep the "old" API and handle it in code but we would really appreciate if we could avoid this, since it just adds complexity in code/more tests for something that nobody would ever use. Maybe we can reconsider? Apologies for the odd situation here.

htuch · 2024-03-27T04:53:57Z

Ack. I think we can make an exception here then, @wbpcode @envoyproxy/api-shepherds please raise any concerns on allowing a breaking change out of the window given the context in #33034 (comment)

wbpcode · 2024-03-27T06:02:43Z

I am OK to make it an exception. :)

… a new" This reverts commit cec4b0f. Signed-off-by: thomas.ebner <[email protected]>

samohte

Thanks, I have reverted cec4b0f where I started to add handling for the old config.

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto

wbpcode · 2024-03-27T06:46:17Z

source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc

-      message->headers().setReference(Http::CustomHeaders::get().Authorization,
-                                      authorization_header_value_);
+      for (const auto& header_pair : parsed_headers_to_add_) {
+        message->headers().setReference(header_pair.first, header_pair.second);
+      }


LGTM overall except this one. Basically, we think the token or passwd should be kept in secret. And if an raw string is used then we should mark it as sensitive and the config_dump will hide it when dump all the configurations.

So, if you use the request_headers_to_add, the token may be exposed in the dumped configuration file.

If you think it's OK, then I am also OK to this design.

Thank's for the hint. I have discussed this with @joaopgrassi. We are aware of the problem but we would like to keep it as is for now. We would have to add the [(udpa.annotations.sensitive) = true] annotation to the http_servicefield. This would make it very hard to debug a wrong config.

My suggestion is to keep the token in another independent field and mark it as sensitive. But if you guys think the current implementation is OK. Then I am happy to approve.

Waiting your last confirmation, then I will merge this PR.

/wait-any

@wbpcode We have discussed internally and we prefer to keep it as is. We already have the same situation in the HTTP exporter, where the token is also passed/used in the http headers, so we prefer to keep the config consistent to not confuse users. It also makes the Istio part of the configuration easier, because we can then re-use the headers of the HTTP exporter for the sampler, as in 99% of the cases they will be configured together by customers (http exporter + Dynatrace sampler).

wbpcode · 2024-03-28T01:47:47Z

/lgtm

repokitteh-read-only · 2024-03-28T01:47:51Z

please specify a single label can be specified

🐱

Caused by: a #33034 (comment) was created by @wbpcode.

see: more, trace.

envoyproxy#33034) * opentelemetrytracer: Dynatrace sampler: Use http_service in configuration. (#22) * change dynatrace sampler config to use http_service Signed-off-by: thomas.ebner <[email protected]> Signed-off-by: Thomas Ebner <[email protected]> Co-authored-by: Joao Grassi <[email protected]> * modify protobuf doc Signed-off-by: thomas.ebner <[email protected]> * remove empty line in proto file Signed-off-by: thomas.ebner <[email protected]> * added empty line in proto file Signed-off-by: thomas.ebner <[email protected]> * try to fix "Error in "code-block" directive" Signed-off-by: thomas.ebner <[email protected]> * fix "Error in "code-block" directive" Signed-off-by: thomas.ebner <[email protected]> * change proto documentation again Signed-off-by: thomas.ebner <[email protected]> * review feedback: keep unused values marked as deprecated. Add a new field. Signed-off-by: thomas.ebner <[email protected]> * Revert "review feedback: keep unused values marked as deprecated. Add a new" This reverts commit cec4b0f. Signed-off-by: thomas.ebner <[email protected]> --------- Signed-off-by: thomas.ebner <[email protected]> Signed-off-by: Thomas Ebner <[email protected]> Co-authored-by: Joao Grassi <[email protected]>

opentelemetrytracer: Dynatrace sampler: Use http_service in configura…

815fc9c

…tion. (#22) * change dynatrace sampler config to use http_service Signed-off-by: thomas.ebner <[email protected]> Signed-off-by: Thomas Ebner <[email protected]> Co-authored-by: Joao Grassi <[email protected]>

repokitteh-read-only bot added the api label Mar 21, 2024

repokitteh-read-only bot assigned htuch Mar 21, 2024

samohte added 6 commits March 21, 2024 16:37

Merge remote-tracking branch 'upstream/main' into feat/use-http-servi…

2aaa593

…ce-to-configure-dynatrace-sampler Signed-off-by: thomas.ebner <[email protected]>

modify protobuf doc

7d1a86a

Signed-off-by: thomas.ebner <[email protected]>

remove empty line in proto file

8fc17d4

Signed-off-by: thomas.ebner <[email protected]>

added empty line in proto file

9198d06

Signed-off-by: thomas.ebner <[email protected]>

try to fix "Error in "code-block" directive"

9c1e750

Signed-off-by: thomas.ebner <[email protected]>

fix "Error in "code-block" directive"

b819ee8

Signed-off-by: thomas.ebner <[email protected]>

samohte marked this pull request as ready for review March 21, 2024 19:24

samohte requested a review from htuch as a code owner March 21, 2024 19:24

change proto documentation again

087f921

Signed-off-by: thomas.ebner <[email protected]>

repokitteh-read-only bot added the waiting label Mar 22, 2024

wbpcode reviewed Mar 22, 2024

View reviewed changes

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto Show resolved Hide resolved

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto Show resolved Hide resolved

review feedback: keep unused values marked as deprecated. Add a new

cec4b0f

field. Signed-off-by: thomas.ebner <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 25, 2024

wbpcode reviewed Mar 25, 2024

View reviewed changes

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto Outdated Show resolved Hide resolved

source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc Show resolved Hide resolved

Revert "review feedback: keep unused values marked as deprecated. Add…

08600a1

… a new" This reverts commit cec4b0f. Signed-off-by: thomas.ebner <[email protected]>

samohte commented Mar 27, 2024

View reviewed changes

api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto Show resolved Hide resolved

wbpcode reviewed Mar 27, 2024

View reviewed changes

wbpcode approved these changes Mar 28, 2024

View reviewed changes

repokitteh-read-only bot added waiting:any and removed api waiting:any labels Mar 28, 2024

wbpcode merged commit 838f272 into envoyproxy:main Mar 28, 2024
53 checks passed

samohte deleted the feat/use-http-service-to-configure-dynatrace-sampler branch March 28, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opentelemetrytracer: Dynatrace sampler: Use http_service in configura… #33034

opentelemetrytracer: Dynatrace sampler: Use http_service in configura… #33034

samohte commented Mar 21, 2024

repokitteh-read-only bot commented Mar 21, 2024

repokitteh-read-only bot commented Mar 21, 2024

wbpcode commented Mar 22, 2024

joaopgrassi commented Mar 22, 2024

wbpcode commented Mar 22, 2024

wbpcode commented Mar 22, 2024

htuch commented Mar 26, 2024

joaopgrassi commented Mar 26, 2024 •

edited

Loading

htuch commented Mar 27, 2024

wbpcode commented Mar 27, 2024 •

edited

Loading

samohte left a comment

wbpcode Mar 27, 2024 •

edited

Loading

samohte Mar 27, 2024

wbpcode Mar 28, 2024

wbpcode Mar 28, 2024

joaopgrassi Mar 28, 2024 •

edited

Loading

wbpcode commented Mar 28, 2024

repokitteh-read-only bot commented Mar 28, 2024

opentelemetrytracer: Dynatrace sampler: Use http_service in configura… #33034

opentelemetrytracer: Dynatrace sampler: Use http_service in configura… #33034

Conversation

samohte commented Mar 21, 2024

repokitteh-read-only bot commented Mar 21, 2024

repokitteh-read-only bot commented Mar 21, 2024

wbpcode commented Mar 22, 2024

joaopgrassi commented Mar 22, 2024

wbpcode commented Mar 22, 2024

wbpcode commented Mar 22, 2024

htuch commented Mar 26, 2024

joaopgrassi commented Mar 26, 2024 • edited Loading

htuch commented Mar 27, 2024

wbpcode commented Mar 27, 2024 • edited Loading

samohte left a comment

Choose a reason for hiding this comment

wbpcode Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

samohte Mar 27, 2024

Choose a reason for hiding this comment

wbpcode Mar 28, 2024

Choose a reason for hiding this comment

wbpcode Mar 28, 2024

Choose a reason for hiding this comment

joaopgrassi Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Mar 28, 2024

repokitteh-read-only bot commented Mar 28, 2024

joaopgrassi commented Mar 26, 2024 •

edited

Loading

wbpcode commented Mar 27, 2024 •

edited

Loading

wbpcode Mar 27, 2024 •

edited

Loading

joaopgrassi Mar 28, 2024 •

edited

Loading