-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
opentelemetrytracer: Added support to configure a Dynatrace sampler #32598
opentelemetrytracer: Added support to configure a Dynatrace sampler #32598
Conversation
Signed-off-by: Thomas Ebner <[email protected]> Signed-off-by: Joao Grassi <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
SamplerConfigProviderImpl::SamplerConfigProviderImpl( | ||
Server::Configuration::TracerFactoryContext& /*context*/, | ||
const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config) | ||
: sampler_config_(config.root_spans_per_minute()) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our goal is that sampling configuration is obtained in the background from the Dynatrace API, to achieve dynamic sampling.
For this PR we went with this simple approach of returning the value from the configuration. The fetching from the API will be added in a follow up PR.
Signed-off-by: Joao Grassi <[email protected]>
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: Joao Grassi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be literals - so double backticks please
api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Joao Grassi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skimmed at a high level and seems fine. Given this is a new opt-in WIP feature I think it's ok to merge and see what issues come up for people that use the feature. Thanks!
Signed-off-by: Joao Grassi <[email protected]>
Head branch was pushed to by a user without write access
@mattklein123 there was a conflict with the changelog.. solved it now :) |
/retest |
Working to improve the code coverage a bit |
Signed-off-by: thomas.ebner <[email protected]>
Head branch was pushed to by a user without write access
@mattklein123 need the approval again, sorry. There was issues with code coverage, but we added more tests and should work now. Thanks! |
Signed-off-by: Joao Grassi <[email protected]>
Signed-off-by: thomas.ebner <[email protected]>
Signed-off-by: thomas.ebner <[email protected]>
Commit Message: Added support to configure a Dynatrace sampler
Additional Description: Given the OpenTracing deprecation, Dynatrace customers need a solution to migrate to the OpenTelememetry tracer in Envoy. We have contributed several features needed and the sampler is the last step. Please refer to the linked issue for more information/context.
Risk Level: Low
Testing: Unit, Integration, Manual
Docs Changes: Proto docs generated and verified locally
Release Notes: Added support to configure a Dynatrace sampler
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #32581]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]