[SDK] Add tracer scope configurator #3137

psx95 · 2024-11-12T23:37:51Z

Adds scope configurator for Tracers.

Changes

Updates the Trace SDK to add support for TracerConfigurator as per the spec changes.
Adds a TracerConfig class that can be used to configure a tracer's behavior as per spec.

Some optional, good-to-have convenience functions were recommended by the spec to accommodate common use-cases - these have not been added in this PR.

See the TracerConfigurator spec for these recommendations. They should be added in a future PR.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

netlify · 2024-11-12T23:38:07Z

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	`2329279`
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/673ba561a4a91f00082fc899

codecov · 2024-11-12T23:55:05Z

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.81%. Comparing base (a388e87) to head (2329279).

Files with missing lines	Patch %	Lines
sdk/src/trace/tracer_provider_factory.cc	50.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
- Coverage   87.83%   87.81%   -0.02%     
==========================================
  Files         195      197       +2     
  Lines        6146     6176      +30     
==========================================
+ Hits         5398     5423      +25     
- Misses        748      753       +5

Files with missing lines	Coverage Δ
...y/sdk/instrumentationscope/instrumentation_scope.h	`100.00% <ø> (ø)`
sdk/include/opentelemetry/sdk/trace/tracer.h	`100.00% <ø> (ø)`
...dk/include/opentelemetry/sdk/trace/tracer_config.h	`100.00% <100.00%> (ø)`
...k/include/opentelemetry/sdk/trace/tracer_context.h	`100.00% <ø> (ø)`
sdk/src/trace/tracer.cc	`83.34% <100.00%> (+0.88%)`	⬆️
sdk/src/trace/tracer_config.cc	`100.00% <100.00%> (ø)`
sdk/src/trace/tracer_context.cc	`84.00% <100.00%> (+2.19%)`	⬆️
sdk/src/trace/tracer_provider.cc	`89.14% <100.00%> (ø)`
sdk/src/trace/tracer_provider_factory.cc	`46.16% <50.00%> (+1.33%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

marcalff · 2024-11-21T22:21:15Z

sdk/include/opentelemetry/sdk/trace/tracer.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_;


Replace with:

TracerConfig tracer_config_;

There is no need to allocate memory and use a unique pointer, the tracer can own it's own configuration directly.

marcalff · 2024-11-21T22:23:50Z

sdk/src/trace/tracer.cc

+const std::shared_ptr<opentelemetry::trace::NoopTracer> Tracer::kNoopTracer =
+    std::make_shared<opentelemetry::trace::NoopTracer>();


Why a shared_ptr ?

This memory will never be reclaimed.

This should be enough:

opentelemetry::trace::NoopTracer kNoopTracer();

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.

marcalff · 2024-11-21T22:27:55Z

sdk/include/opentelemetry/sdk/trace/tracer_config.h

+  /**
+   * 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();


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.

marcalff · 2024-11-21T22:28:57Z

sdk/include/opentelemetry/sdk/trace/tracer_config.h

+  bool disabled_;
+  static const TracerConfig kDefaultConfig;
+  static const TracerConfig kDisabledConfig;
+  static const instrumentationscope::ScopeConfigurator<TracerConfig> kDefaultTracerConfigurator;


Remove kDefaultTracerConfigurator.

marcalff · 2024-11-21T22:42:34Z

sdk/include/opentelemetry/sdk/instrumentationscope/instrumentation_scope.h

+template <class T>
+using ScopeConfigurator = std::function<T(const InstrumentationScope &)>;


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.

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.

In the mean time, the spec configuration repository is defining the yaml bits for this:

Add scope configuration properties opentelemetry-configuration#140

Instead of "AlwaysOn" / "AlwaysOff", the following is probably better:

class ScopeConfigurator;
class DefaultScopeConfigurator : public ScopeConfigurator;

and later

class NamedScopeConfigurator : public ScopeConfigurator; (for name matching)

marcalff · 2024-11-21T22:45:02Z

sdk/include/opentelemetry/sdk/trace/tracer_context.h

+      std::unique_ptr<instrumentationscope::ScopeConfigurator<TracerConfig>> tracer_configurator =
+          std::make_unique<instrumentationscope::ScopeConfigurator<TracerConfig>>(
+              TracerConfig::DefaultConfigurator())) noexcept;


Replace with (assuming the default is enabled):

tracer_configurator = AlwaysOnScopeConfigurator::Create();

Likewise in other places.

marcalff · 2024-11-21T22:45:45Z

sdk/include/opentelemetry/sdk/trace/tracer_context.h

@@ -77,6 +83,8 @@ class TracerContext
   */
  const opentelemetry::sdk::resource::Resource &GetResource() const noexcept;

+  instrumentationscope::ScopeConfigurator<TracerConfig> &GetTracerConfigurator() const noexcept;


The return type should probably be a const reference, not just a reference.

marcalff · 2024-11-21T22:50:16Z

sdk/src/trace/tracer.cc

+    if (!tracer_config_->IsEnabled())
+    {
+      return kNoopTracer->StartSpan(name, attributes, links, options);
+    }


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.

marcalff · 2024-11-21T22:56:16Z

sdk/include/opentelemetry/sdk/trace/tracer_config.h

+  static const instrumentationscope::ScopeConfigurator<TracerConfig> &DefaultConfigurator();
+
+private:
+  explicit TracerConfig(const bool disabled = false) : disabled_(disabled) {}


Do not use default values = false.

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.

This is a private constructor, so it cannot be called externally and the value of the default is dictated by the spec.

marcalff

Thanks for the patch.

This is a very good start, see comments to refine the structure around TracerConfig + ScopeConfigurator.

psx95 · 2024-11-21T23:45:06Z

Thanks for the detailed review, I will go over the suggestions and make necessary changes.

psx95 added 2 commits November 8, 2024 22:38

Add tracer_config as per spec

229a93c

Update formatting

73df243

psx95 force-pushed the add-scope-config branch from 6e15bc3 to 75d84d8 Compare November 12, 2024 23:43

TracerProvider uses TracerConfig as per spec

fd0178c

psx95 force-pushed the add-scope-config branch from 75d84d8 to fd0178c Compare November 13, 2024 01:02

psx95 added 2 commits November 13, 2024 11:51

Merge branch 'main' into add-scope-config

ab6b2c4

Fix iwyu warnings generated

606ae37

psx95 force-pushed the add-scope-config branch from ec4209e to 606ae37 Compare November 13, 2024 19:58

psx95 added 5 commits November 13, 2024 14:59

Merge branch 'main' into add-scope-config

f9d9815

Fix formatting

9cd78f1

Replace scope_configurator class with typedef

2a97de9

Update factories to accept TracerConfigurator

a5e6691

Fix IWYU warnings

fbfd3de

psx95 force-pushed the add-scope-config branch from f5a5673 to fbfd3de Compare November 14, 2024 18:24

psx95 added 4 commits November 14, 2024 13:24

Merge branch 'main' into add-scope-config

c775d8f

Add operator overload to check for equality

f22f9ec

Add tests for TracerConfig

355550a

Fix formatting

4aadb2a

psx95 force-pushed the add-scope-config branch 2 times, most recently from fee7f7c to 15274c0 Compare November 15, 2024 00:09

Fix memory leak issue in test

e02d1c5

psx95 force-pushed the add-scope-config branch from 15274c0 to e02d1c5 Compare November 15, 2024 00:20

Replace typedef with type alias

a8a04e5

psx95 force-pushed the add-scope-config branch from 28aae45 to a8a04e5 Compare November 15, 2024 16:04

Infer trace_config from the context

79c32f3

psx95 force-pushed the add-scope-config branch from 5517d8a to 79c32f3 Compare November 15, 2024 20:01

Merge branch 'main' into add-scope-config

db8d879

psx95 force-pushed the add-scope-config branch from fc6e563 to 85ce799 Compare November 16, 2024 16:32

psx95 changed the title ~~[WIP] Add scope config~~ [WIP] Add tracer scope configurator Nov 16, 2024

psx95 force-pushed the add-scope-config branch from c8640fb to 6e8a308 Compare November 16, 2024 18:54

Fix iwyu warnings

2c55339

psx95 force-pushed the add-scope-config branch 8 times, most recently from 402ba99 to 93950bf Compare November 18, 2024 19:13

Add unit test for custom configurator

c77065e

psx95 force-pushed the add-scope-config branch from 93950bf to c77065e Compare November 18, 2024 20:02

psx95 added 2 commits November 18, 2024 20:18

Add documentation for the tracer_config.h

d666e26

Update Changelog

2329279

psx95 changed the title ~~[WIP] Add tracer scope configurator~~ [SDK] Add tracer scope configurator Nov 18, 2024

psx95 marked this pull request as ready for review November 18, 2024 21:09

psx95 requested a review from a team as a code owner November 18, 2024 21:09

marcalff self-requested a review November 18, 2024 21:46

marcalff reviewed Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] Add tracer scope configurator #3137

[SDK] Add tracer scope configurator #3137

psx95 commented Nov 12, 2024 •

edited

Loading

netlify bot commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

marcalff Nov 21, 2024

marcalff Nov 21, 2024

psx95 Nov 21, 2024

marcalff Nov 21, 2024

marcalff Nov 21, 2024

marcalff Nov 21, 2024

psx95 Nov 21, 2024

marcalff Nov 23, 2024

marcalff Nov 21, 2024 •

edited

Loading

marcalff Nov 21, 2024

marcalff Nov 21, 2024

marcalff Nov 21, 2024

psx95 Nov 21, 2024

marcalff left a comment

psx95 commented Nov 21, 2024

		const std::shared_ptr<opentelemetry::trace::NoopTracer> Tracer::kNoopTracer =
		std::make_shared<opentelemetry::trace::NoopTracer>();

		template <class T>
		using ScopeConfigurator = std::function<T(const InstrumentationScope &)>;

[SDK] Add tracer scope configurator #3137

Are you sure you want to change the base?

[SDK] Add tracer scope configurator #3137

Conversation

psx95 commented Nov 12, 2024 • edited Loading

Changes

netlify bot commented Nov 12, 2024 • edited Loading

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcalff Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcalff left a comment

Choose a reason for hiding this comment

psx95 commented Nov 21, 2024

psx95 commented Nov 12, 2024 •

edited

Loading

netlify bot commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

marcalff Nov 21, 2024 •

edited

Loading