Skip to content

OpenTelemetryTracingProvider: Allow configuring samplers#3134

Merged
istio-testing merged 6 commits intoistio:masterfrom
dynatrace-oss-contrib:dynatrace-sampler
Mar 28, 2024
Merged

OpenTelemetryTracingProvider: Allow configuring samplers#3134
istio-testing merged 6 commits intoistio:masterfrom
dynatrace-oss-contrib:dynatrace-sampler

Conversation

@joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Mar 20, 2024

API part of istio/istio#50001

The OpenTelemetry project defines a Sampler API and this API is available now in Envoy. With the next Envoy release, it will also come with a Dynatrace Sampler. This API in Envoy allows anyone (vendors for ex) to add their own sampler.

This PR then adds this new configuration on Istio, containing for now the Dynatrace sampler config.

Today, there's multiple ways to configure sampling, which works/have the following precedence:

telemetry.RandomSamplingPercentage > defaultConfig.tracing.sampling > PILOT_TRACE_SAMPLING

This PR adds another option, changing the order above to:

provider.sampler > telemetry.RandomSamplingPercentage > defaultConfig.tracing.sampling > PILOT_TRACE_SAMPLING

Which means:

  • If OpenTelemetryTracingProvider has a custom sampler configured (what this PR adds):

    • Set telemetry.RandomSamplingPercentage to 100 as all spans should arrive on the sampler, so it can perform its algorithm. This is hardcoded and the user cannot change (doesn't make sense to). Without all spans a sampler can't make consistent sampling decisions.
  • Else, continue with the same precedence as today. Essentially nothing changes when users don't use a Sampler:

 telemetry.RandomSamplingPercentage > defaultConfig.tracing.sampling > PILOT_TRACE_SAMPLING

Note: I didn't add the AlwaysOn since that is basically achieved the same way by setting the telemetry.RandomSamplingPercentage to 100.

CC @howardjohn @zirain

Add API configurations for the Dynatrace sampler.
@joaopgrassi joaopgrassi requested a review from a team as a code owner March 20, 2024 15:14
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 20, 2024
//
// When not provided, the Dynatrace Sampler will re-use the configuration from the OpenTelemetryTracingProvider HTTP Exporter
// (`service`, `port` and `http`), including the access token.
DynatraceApi http_service = 4;
Copy link
Member Author

@joaopgrassi joaopgrassi Mar 20, 2024

Choose a reason for hiding this comment

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

The idea behind this optional field is: Dynatrace customers use the OpenTelemetry collector as a central place for processing the spans before sending it to Dynatrace (removing PII from attributes, adding attributes, batching, etc).

In this case, Istio/Envoy can be configured to export to the Collector via gRPC, but still use the Dynatrace Sampler to sample the spans properly before exporting to the collector.

When the Dynatrace customer doesn't use the collector and prefers to send the spans directly to the Dynatrace API, then the Sampler simply re-use the config from the HTTP exporter (service/host, http headers and etc). This is so the configuration stays minimal as most things would be just repeated in this case.

Copy link
Member

Choose a reason for hiding this comment

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

I think it confuses if this is set under opentelemetry provider

Copy link
Member

Choose a reason for hiding this comment

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

it's fine to place under DynatraceSampler, a vendor extension.

Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM

// REQUIRED. The Dynatrace tenant.
//
// The value can be obtained from the Istio deployment page in Dynatrace.
string tenant = 1;
Copy link
Member

Choose a reason for hiding this comment

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

what's this mean

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a Dynatrace specific field. Each Dynatrace customer receives a "Tenant Id" and that is what this field is for. It identifies the customer's tenant in our SaaS product.

@joaopgrassi
Copy link
Member Author

joaopgrassi commented Mar 28, 2024

@howardjohn could you please take a look? This is the sampling PR we talked a while ago. #3002 (comment).

It would be really great if we can get this merged so it can be part of the 1.22 release. I already have the Istio code ready, just waiting for this to open the feature PR. Thank you!

@istio-testing istio-testing merged commit e5b8f83 into istio:master Mar 28, 2024
@joaopgrassi joaopgrassi deleted the dynatrace-sampler branch March 28, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants