-
Notifications
You must be signed in to change notification settings - Fork 427
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
[SDK] Add tracer scope configurator #3137
base: main
Are you sure you want to change the base?
Changes from all commits
229a93c
73df243
fd0178c
ab6b2c4
606ae37
f9d9815
9cd78f1
2a97de9
a5e6691
fbfd3de
c775d8f
f22f9ec
355550a
4aadb2a
e02d1c5
a8a04e5
79c32f3
db8d879
f6ff444
2c55339
c77065e
d666e26
2329279
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,9 @@ | |
#include "opentelemetry/sdk/trace/id_generator.h" | ||
#include "opentelemetry/sdk/trace/processor.h" | ||
#include "opentelemetry/sdk/trace/sampler.h" | ||
#include "opentelemetry/sdk/trace/tracer_config.h" | ||
#include "opentelemetry/sdk/trace/tracer_context.h" | ||
#include "opentelemetry/trace/noop.h" | ||
#include "opentelemetry/trace/span.h" | ||
#include "opentelemetry/trace/span_context_kv_iterable.h" | ||
#include "opentelemetry/trace/span_startoptions.h" | ||
|
@@ -103,8 +105,10 @@ class Tracer final : public opentelemetry::trace::Tracer, | |
private: | ||
// order of declaration is important here - instrumentation scope should destroy after | ||
// tracer-context. | ||
std::unique_ptr<TracerConfig> tracer_config_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace with:
There is no need to allocate memory and use a unique pointer, the tracer can own it's own configuration directly. |
||
std::shared_ptr<InstrumentationScope> instrumentation_scope_; | ||
std::shared_ptr<TracerContext> context_; | ||
static const std::shared_ptr<opentelemetry::trace::NoopTracer> kNoopTracer; | ||
}; | ||
} // namespace trace | ||
} // namespace sdk | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
#pragma once | ||
|
||
#include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" | ||
#include "opentelemetry/version.h" | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
namespace sdk | ||
{ | ||
namespace trace | ||
{ | ||
/** | ||
* TracerConfig defines various configurable aspects of a Tracer's behavior. | ||
* This class should not be used directly to configure a Tracer's behavior, instead a | ||
* ScopeConfigurator should be used to compute the desired TracerConfig which can then be used to | ||
* configure a Tracer. | ||
*/ | ||
class TracerConfig | ||
{ | ||
public: | ||
bool operator==(const TracerConfig &other) const noexcept; | ||
|
||
/** | ||
* Returns if the Tracer is enabled or disabled. Tracers are enabled by default. | ||
* @return a boolean indicating if the Tracer is enabled. Defaults to true. | ||
*/ | ||
bool IsEnabled() const noexcept; | ||
|
||
/** | ||
* Returns a TracerConfig that represents a disabled Tracer. A disabled tracer behaves like a | ||
* no-op tracer. | ||
* @return a static constant TracerConfig that represents a disabled tracer. | ||
*/ | ||
static TracerConfig Disabled(); | ||
|
||
/** | ||
* Returns a TracerConfig that represents an enabled Tracer. | ||
* @return a static constant TracerConfig that represents an enabled tracer. | ||
*/ | ||
static TracerConfig Enabled(); | ||
|
||
/** | ||
* Returns a TracerConfig that represents a Tracer configured with the default behavior. | ||
* The default behavior is guided by the OpenTelemetry specification. | ||
* @return a static constant TracerConfig that represents a tracer configured with default | ||
* behavior. | ||
*/ | ||
static TracerConfig Default(); | ||
|
||
/** | ||
* Returns a ScopeConfigurator that can be used to get the default tracer configurator. | ||
* A tracer configurator computes TracerConfig based on the InstrumentationScope. The default | ||
* tracer configurator unconditionally computes the default TracerConfig for all scopes. | ||
* | ||
* @return a static constant TracerConfig that represents a tracer configurator with default | ||
* behavior. | ||
*/ | ||
static const instrumentationscope::ScopeConfigurator<TracerConfig> &DefaultConfigurator(); | ||
Comment on lines
+52
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To move in a separate class. A TracerConfig represents a configuration. This class should not know how / when / by whom a configuration is created, and should not even know of ScopeConfigurator. |
||
|
||
private: | ||
explicit TracerConfig(const bool disabled = false) : disabled_(disabled) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not use default values The choice of the default value should not be nested deep in the code, but bubble up in the call stack, up to where the decision is made to use a default. This will be in the tracer provider factory, when no scope configurator is provided. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a private constructor, so it cannot be called externally and the value of the default is dictated by the spec. |
||
bool disabled_; | ||
static const TracerConfig kDefaultConfig; | ||
static const TracerConfig kDisabledConfig; | ||
static const instrumentationscope::ScopeConfigurator<TracerConfig> kDefaultTracerConfigurator; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove kDefaultTracerConfigurator. |
||
}; | ||
} // namespace trace | ||
} // namespace sdk | ||
OPENTELEMETRY_END_NAMESPACE |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#include "opentelemetry/sdk/trace/random_id_generator.h" | ||
#include "opentelemetry/sdk/trace/sampler.h" | ||
#include "opentelemetry/sdk/trace/samplers/always_on.h" | ||
#include "opentelemetry/sdk/trace/tracer_config.h" | ||
#include "opentelemetry/version.h" | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
|
@@ -21,6 +22,8 @@ namespace sdk | |
namespace trace | ||
{ | ||
|
||
using namespace opentelemetry::sdk::instrumentationscope; | ||
|
||
/** | ||
* A class which stores the TracerProvider context. | ||
* | ||
|
@@ -43,7 +46,10 @@ class TracerContext | |
opentelemetry::sdk::resource::Resource::Create({}), | ||
std::unique_ptr<Sampler> sampler = std::unique_ptr<AlwaysOnSampler>(new AlwaysOnSampler), | ||
std::unique_ptr<IdGenerator> id_generator = | ||
std::unique_ptr<IdGenerator>(new RandomIdGenerator())) noexcept; | ||
std::unique_ptr<IdGenerator>(new RandomIdGenerator()), | ||
std::unique_ptr<instrumentationscope::ScopeConfigurator<TracerConfig>> tracer_configurator = | ||
std::make_unique<instrumentationscope::ScopeConfigurator<TracerConfig>>( | ||
TracerConfig::DefaultConfigurator())) noexcept; | ||
Comment on lines
+50
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace with (assuming the default is enabled): tracer_configurator = AlwaysOnScopeConfigurator::Create(); Likewise in other places. |
||
|
||
virtual ~TracerContext() = default; | ||
|
||
|
@@ -77,6 +83,8 @@ class TracerContext | |
*/ | ||
const opentelemetry::sdk::resource::Resource &GetResource() const noexcept; | ||
|
||
instrumentationscope::ScopeConfigurator<TracerConfig> &GetTracerConfigurator() const noexcept; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return type should probably be a const reference, not just a reference. |
||
|
||
/** | ||
* Obtain the Id Generator associated with this tracer context. | ||
* @return The ID Generator for this tracer context. | ||
|
@@ -100,6 +108,7 @@ class TracerContext | |
std::unique_ptr<Sampler> sampler_; | ||
std::unique_ptr<IdGenerator> id_generator_; | ||
std::unique_ptr<SpanProcessor> processor_; | ||
std::unique_ptr<instrumentationscope::ScopeConfigurator<TracerConfig>> tracer_configurator_; | ||
}; | ||
|
||
} // namespace trace | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
|
||
#include <stdint.h> | ||
#include <chrono> | ||
#include <functional> | ||
#include <map> | ||
#include <new> | ||
#include <utility> | ||
|
@@ -16,6 +17,7 @@ | |
#include "opentelemetry/sdk/trace/id_generator.h" | ||
#include "opentelemetry/sdk/trace/sampler.h" | ||
#include "opentelemetry/sdk/trace/tracer.h" | ||
#include "opentelemetry/sdk/trace/tracer_config.h" | ||
#include "opentelemetry/sdk/trace/tracer_context.h" | ||
#include "opentelemetry/trace/context.h" | ||
#include "opentelemetry/trace/noop.h" | ||
|
@@ -36,11 +38,16 @@ namespace sdk | |
{ | ||
namespace trace | ||
{ | ||
const std::shared_ptr<opentelemetry::trace::NoopTracer> Tracer::kNoopTracer = | ||
std::make_shared<opentelemetry::trace::NoopTracer>(); | ||
Comment on lines
+41
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why a shared_ptr ? This memory will never be reclaimed. This should be enough:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to avoid making multiple copies of NoopTracer, but looking at it again, shared_ptr might actually have more overhead due to ref counts. |
||
|
||
Tracer::Tracer(std::shared_ptr<TracerContext> context, | ||
std::unique_ptr<InstrumentationScope> instrumentation_scope) noexcept | ||
: instrumentation_scope_{std::move(instrumentation_scope)}, context_{std::move(context)} | ||
{} | ||
{ | ||
tracer_config_ = | ||
std::make_unique<TracerConfig>(context_->GetTracerConfigurator()(*instrumentation_scope_)); | ||
} | ||
|
||
nostd::shared_ptr<opentelemetry::trace::Span> Tracer::StartSpan( | ||
nostd::string_view name, | ||
|
@@ -51,6 +58,10 @@ nostd::shared_ptr<opentelemetry::trace::Span> Tracer::StartSpan( | |
opentelemetry::trace::SpanContext parent_context = GetCurrentSpan()->GetContext(); | ||
if (nostd::holds_alternative<opentelemetry::trace::SpanContext>(options.parent)) | ||
{ | ||
if (!tracer_config_->IsEnabled()) | ||
{ | ||
return kNoopTracer->StartSpan(name, attributes, links, options); | ||
} | ||
Comment on lines
+61
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my understanding, this should be moved earlier, not done only for the SpanContext alternative. If the tracer is disabled, it is disabled for all types of spans. |
||
auto span_context = nostd::get<opentelemetry::trace::SpanContext>(options.parent); | ||
if (span_context.IsValid()) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
#include "opentelemetry/sdk/trace/tracer_config.h" | ||
#include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
namespace sdk | ||
{ | ||
namespace trace | ||
{ | ||
|
||
const TracerConfig TracerConfig::kDefaultConfig = TracerConfig(); | ||
const TracerConfig TracerConfig::kDisabledConfig = TracerConfig(true); | ||
|
||
const instrumentationscope::ScopeConfigurator<TracerConfig> | ||
TracerConfig::kDefaultTracerConfigurator = | ||
[](const instrumentationscope::InstrumentationScope &) { return Default(); }; | ||
|
||
TracerConfig TracerConfig::Disabled() | ||
{ | ||
return kDisabledConfig; | ||
} | ||
|
||
TracerConfig TracerConfig::Enabled() | ||
{ | ||
return kDefaultConfig; | ||
} | ||
|
||
TracerConfig TracerConfig::Default() | ||
{ | ||
return kDefaultConfig; | ||
} | ||
|
||
const instrumentationscope::ScopeConfigurator<TracerConfig> &TracerConfig::DefaultConfigurator() | ||
{ | ||
return kDefaultTracerConfigurator; | ||
} | ||
|
||
bool TracerConfig::IsEnabled() const noexcept | ||
{ | ||
return !disabled_; | ||
} | ||
|
||
bool TracerConfig::operator==(const TracerConfig &other) const noexcept | ||
{ | ||
return disabled_ == other.disabled_; | ||
} | ||
|
||
} // namespace trace | ||
} // namespace sdk | ||
OPENTELEMETRY_END_NAMESPACE |
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.
A couple of comments.
(1)
ScopeConfigurator should be an abstract class, with a virtual configure() method.
This will help to define different kinds of configurators later, including the various builtins helpers.
(2)
One of the configurators is the always enabled one, so implement a AlwaysOnScopeConfigurator sub class.
Have a static AlwaysOnScopeConfigurator::Create() method, to return an instance.
(3)
Likewise, AlwaysOffScopeConfigurator::Create()
(4)
All this should be in dedicated headers, so instrumentation_scope.h should be unchanged.
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.
Sounds good, I had a class before, but removed since it was easily replaceable by a function. I was expecting to put the built-in helpers in a static utility class, but this approach might look better.
I will give this a try.
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.
In the mean time, the spec configuration repository is defining the yaml bits for this:
Instead of "AlwaysOn" / "AlwaysOff", the following is probably better:
class ScopeConfigurator;
class DefaultScopeConfigurator : public ScopeConfigurator;
and later
class NamedScopeConfigurator : public ScopeConfigurator; (for name matching)