Skip to content

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

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.

Sign up for GitHub

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

Merged
wbpcode merged 10 commits into from
Mar 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
815fc9c
opentelemetrytracer: Dynatrace sampler: Use http_service in configura…
samohte Mar 21, 2024
2aaa593
Merge remote-tracking branch 'upstream/main' into feat/use-http-servi…
samohte Mar 21, 2024
7d1a86a
modify protobuf doc
samohte Mar 21, 2024
8fc17d4
remove empty line in proto file
samohte Mar 21, 2024
9198d06
added empty line in proto file
samohte Mar 21, 2024
9c1e750
try to fix "Error in "code-block" directive"
samohte Mar 21, 2024
b819ee8
fix "Error in "code-block" directive"
samohte Mar 21, 2024
087f921
change proto documentation again
samohte Mar 22, 2024
cec4b0f
review feedback: keep unused values marked as deprecated. Add a new
samohte Mar 25, 2024
08600a1
Revert "review feedback: keep unused values marked as deprecated. Add…
samohte Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/envoy/extensions/tracers/opentelemetry/samplers/v3/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ licenses(["notice"]) # Apache 2

api_proto_package(
deps = [
"//envoy/annotations:pkg",
"//envoy/config/core/v3:pkg",
"@com_github_cncf_xds//udpa/annotations:pkg",
],
Expand Down
39 changes: 25 additions & 14 deletions api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ syntax = "proto3";

package envoy.extensions.tracers.opentelemetry.samplers.v3;

import "envoy/config/core/v3/http_service.proto";
import "envoy/config/core/v3/http_uri.proto";

import "envoy/annotations/deprecation.proto";
import "udpa/annotations/status.proto";

option java_package = "io.envoyproxy.envoy.extensions.tracers.opentelemetry.samplers.v3";
Expand All @@ -13,10 +15,10 @@ option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/tra
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: Dynatrace Sampler config]

// Configuration for the Dynatrace Sampler extension.
// [#extension: envoy.tracers.opentelemetry.samplers.dynatrace]

// [#next-free-field: 6]
// [#next-free-field: 7]
message DynatraceSamplerConfig {
// The Dynatrace tenant.
//
Expand All @@ -28,19 +30,12 @@ message DynatraceSamplerConfig {
// The value can be obtained from the Envoy deployment page in Dynatrace.
int32 cluster_id = 2;

// The HTTP URI to fetch the sampler configuration (root spans per minute). For example:
//
// .. code-block:: yaml
//
// http_uri:
// uri: <tenant>.dev.dynatracelabs.com/api/v2/samplingConfiguration
// cluster: dynatrace
// timeout: 10s
//
config.core.v3.HttpUri http_uri = 3;
// [#not-implemented-hide:]
config.core.v3.HttpUri http_uri = 3
[deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];
Comment thread
samohte marked this conversation as resolved.
Outdated

// The access token to fetch the sampling configuration from the Dynatrace API
string token = 4;
// [#not-implemented-hide:]
string token = 4 [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];

// Default number of root spans per minute, used when the value can't be obtained from the Dynatrace API.
//
Expand All @@ -50,4 +45,20 @@ message DynatraceSamplerConfig {
// - ``root_spans_per_minute`` is set to 0
//
uint32 root_spans_per_minute = 5;

// The HTTP service to fetch the sampler configuration from the Dynatrace API (root spans per minute). For example:
//
// .. code-block:: yaml
//
// http_service:
// http_uri:
// cluster: dynatrace
// uri: <tenant>.dev.dynatracelabs.com/api/v2/samplingConfiguration
// timeout: 10s
// request_headers_to_add:
// - header:
// key : "authorization"
// value: "Api-Token dt..."
//
config.core.v3.HttpService http_service = 6;
}
1 change: 1 addition & 0 deletions source/extensions/tracers/opentelemetry/samplers/dynatrace/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ envoy_cc_library(
"//source/common/config:datasource_lib",
"//source/extensions/tracers/opentelemetry:opentelemetry_tracer_lib",
"//source/extensions/tracers/opentelemetry/samplers:sampler_lib",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/tracers/opentelemetry/samplers/v3:pkg_cc_proto",
],
)
14 changes: 9 additions & 5 deletions source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ SamplerConfigProviderImpl::SamplerConfigProviderImpl(
Server::Configuration::TracerFactoryContext& context,
const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config)
: cluster_manager_(context.serverFactoryContext().clusterManager()),
http_uri_(config.http_uri()),
authorization_header_value_(absl::StrCat("Api-Token ", config.token())),
sampler_config_(config.root_spans_per_minute()) {
http_uri_(config.http_service().http_uri()), sampler_config_(config.root_spans_per_minute()) {

Comment thread
samohte marked this conversation as resolved.
for (const auto& header_value_option : config.http_service().request_headers_to_add()) {
parsed_headers_to_add_.push_back({Http::LowerCaseString(header_value_option.header().key()),
header_value_option.header().value()});
}

timer_ = context.serverFactoryContext().mainThreadDispatcher().createTimer([this]() -> void {
const auto thread_local_cluster = cluster_manager_.getThreadLocalCluster(http_uri_.cluster());
Expand All @@ -47,8 +50,9 @@ SamplerConfigProviderImpl::SamplerConfigProviderImpl(
} else {
Http::RequestMessagePtr message = Http::Utility::prepareHeaders(http_uri_);
message->headers().setReferenceMethod(Http::Headers::get().MethodValues.Get);
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);
}
Comment on lines -50 to +55

@wbpcode wbpcode Mar 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@samohte samohte Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@wbpcode wbpcode Mar 28, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@wbpcode wbpcode Mar 28, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

/wait-any

@joaopgrassi joaopgrassi Mar 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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).

active_request_ = thread_local_cluster->httpAsyncClient().send(
std::move(message), *this,
Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include <utility>
#include <vector>

#include "envoy/config/core/v3/http_service.pb.h"
#include "envoy/config/core/v3/http_uri.pb.h"
#include "envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.pb.h"
#include "envoy/http/async_client.h"
#include "envoy/http/message.h"
Expand Down Expand Up @@ -65,7 +67,7 @@ class SamplerConfigProviderImpl : public SamplerConfigProvider,
Event::TimerPtr timer_;
Upstream::ClusterManager& cluster_manager_;
envoy::config::core::v3::HttpUri http_uri_;
const std::string authorization_header_value_;
std::vector<std::pair<const Http::LowerCaseString, const std::string>> parsed_headers_to_add_;
Http::AsyncClient::Request* active_request_{};
SamplerConfig sampler_config_;
};
Expand Down
45 changes: 24 additions & 21 deletions test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,15 @@ class SamplerConfigProviderTest : public testing::Test {
const std::string yaml_string = R"EOF(
tenant: "abc12345"
cluster_id: -1743916452
token: "tokenval"
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/api/v2/samplingConfiguration"
timeout: 0.250s
http_service:
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/api/v2/samplingConfiguration"
timeout: 0.250s
request_headers_to_add:
- header:
key: "authorization"
value: "Api-Token tokenval"
root_spans_per_minute: 1000
)EOF";
TestUtility::loadFromYaml(yaml_string, proto_config_);
Expand All @@ -58,7 +62,6 @@ class SamplerConfigProviderTest : public testing::Test {
};

MATCHER_P(MessageMatcher, unusedArg, "") {
// prefix 'Api-Token' should be added to 'tokenval' set via SamplerConfigProvider constructor
return (arg->headers()
.get(Http::CustomHeaders::get().Authorization)[0]
->value()
Expand Down Expand Up @@ -206,11 +209,11 @@ TEST_F(SamplerConfigProviderTest, TestValueConfigured) {
const std::string yaml_string = R"EOF(
tenant: "abc12345"
cluster_id: -1743916452
token: "tokenval"
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/otlp/v1/traces"
timeout: 0.250s
http_service:
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/otlp/v1/traces"
timeout: 0.250s
root_spans_per_minute: 3456
)EOF";

Expand All @@ -226,11 +229,11 @@ TEST_F(SamplerConfigProviderTest, TestNoValueConfigured) {
const std::string yaml_string = R"EOF(
tenant: "abc12345"
cluster_id: -1743916452
token: "tokenval"
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/otlp/v1/traces"
timeout: 500s
http_service:
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/otlp/v1/traces"
timeout: 500s
)EOF";

envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config;
Expand All @@ -246,11 +249,11 @@ TEST_F(SamplerConfigProviderTest, TestValueZeroConfigured) {
const std::string yaml_string = R"EOF(
tenant: "abc12345"
cluster_id: -1743916452
token: "tokenval"
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/otlp/v1/traces"
timeout: 0.250s
http_service:
http_uri:
cluster: "cluster_name"
uri: "https://testhost.com/otlp/v1/traces"
timeout: 0.250s
root_spans_per_minute: 0
)EOF";

Expand Down