Create shared context for updating span pipeline from TracerProvider and affecting Tracer. #650

jsuereth · 2021-04-05T20:15:46Z

Create new TracerContext class that stores (thread-safe) access to Sampler + SpanPrcoessor.
All calls to SpanProcessor go through TracerContext, ensuring we abide by specification where TracerProvider calls MUST impact previously created Tracers
Update all examples for new API.
Adds "stub" API for appending new SpanProcessors to a given TracerContext.

Replaces #590 with only the shared context portion.

…cerProvider reflect to all tracers.

codecov · 2021-04-05T23:05:43Z

Codecov Report

Merging #650 (a0999fc) into main (b0f27a4) will decrease coverage by 0.01%.
The diff coverage is 90.41%.

@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
- Coverage   94.66%   94.65%   -0.02%     
==========================================
  Files         204      206       +2     
  Lines        9418     9381      -37     
==========================================
- Hits         8916     8880      -36     
+ Misses        502      501       -1

Impacted Files	Coverage Δ
sdk/src/trace/span.h	`100.00% <ø> (ø)`
sdk/src/trace/tracer_provider.cc	`76.47% <72.72%> (+8.28%)`	⬆️
sdk/src/trace/tracer_context.cc	`75.00% <75.00%> (ø)`
ext/test/zpages/tracez_data_aggregator_test.cc	`96.35% <100.00%> (ø)`
ext/test/zpages/tracez_processor_test.cc	`98.71% <100.00%> (+<0.01%)`	⬆️
...clude/opentelemetry/sdk/common/atomic_unique_ptr.h	`100.00% <100.00%> (ø)`
sdk/include/opentelemetry/sdk/trace/tracer.h	`100.00% <100.00%> (ø)`
sdk/src/trace/span.cc	`89.58% <100.00%> (ø)`
sdk/src/trace/tracer.cc	`76.92% <100.00%> (+1.92%)`	⬆️
sdk/test/trace/tracer_provider_test.cc	`100.00% <100.00%> (ø)`
... and 5 more

lalitb

LGTM. Thank for the changes.

lalitb · 2021-04-06T10:22:49Z

examples/http/BUILD

+ "//exporters/ostream:ostream_span_exporter",
+ "//sdk/src/trace",
+ ],
+)


Thanks for adding the bazel build for this.

lalitb · 2021-04-06T10:55:01Z

sdk/src/trace/tracer_context.cc

+ // 1. If existing processor is an "AggregateProcessor" append the new processor to it.
+ // 2. If the existing processor is NOT an "AggregateProcessor", create a new Aggregate of this and
+ // the other,
+ // then replace our atomic ptr with the new aggregate.


Though it would be separate PR, just curious how to figure if existing processor is AggregateProcessor :)

Oh, wow, yeah just copy-pasted the documentation. I'll rephrase this for now until the next PR lands.

seemk

I apologize for being out of the loop a bit, but what was the problem this (and #590) solved in the context of Resource SDK? Just to get up to speed, thanks!

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

lalitb · 2021-04-07T14:57:36Z

I apologize for being out of the loop a bit, but what was the problem this (and #590) solved in the context of Resource SDK? Just to get up to speed, thanks!

@seemk - This PR is to ensure that the tracer-provider has sole ownership of all the configuration ( sampler, processor, resources ). You can follow the discussions in #580 specifically these comments:

#580 (comment)
#580 (comment)
#580 (comment) ( the final proposal which is implemented in this PR).

…building client lib.

Migrate needed windows staitc libs to the curl client library and do minor cleanup to builds.

pyohannes

Thanks for doing that work and putting this chunk into its own PR. This makes it much easier to reason about and to review.

pyohannes · 2021-04-09T23:14:37Z

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

+ * Obtain the (conceptual) active processor.
+ *
+ * Note: When more than one processor is active, this will
+ * return an "aggregate" processor
+ */
+ SpanProcessor &GetActiveProcessor() const noexcept;


What would be an inactive processor?

I don't think there is any inactive processor. @jsuereth can confirm though :)

'inactive' would be any constructed but not registered processor.

I'm not tied to this name, just took it from Java ;)

Perhaps just name it GetProcessor to avoid the confusion.

pyohannes · 2021-04-09T23:19:10Z

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

+ * Note: When more than one processor is active, this will
+ * return an "aggregate" processor
+ */
+ SpanProcessor &GetActiveProcessor() const noexcept;


We might theoretically run into issue here when some tracer does some work with a processor obtained by GetActiveProcessor, while another thread calls RegisterPipeline and thus causes the memory pointed to by the obtained the processor to be freed.

Shouldn't be the case. The interface is "RegisterPipeline", and it's a shared pointer. The TracerContext can guarantee that ALL registered processors remain alive for its own lifecycle. I.e. we're not going to delete a pipeline, we're only going to allow registration of new ones specifically to prevent this issue.

We have the inverse problem around Recordable though, specifically if we have an "active" span that did NOT get a recordable registered when a new pipeline is registered, what happens when that span ends?

If you look at my design around "ExportableSpan" where a pipeline can ask for its instance of Recordable (and handle a missing recordable), that's how I planned to solve that issue.

I think I understand. So if a new pipeline is registered, the old one isn't just overwritten, but kept in store and just made inactive?

My understanding was that all the pipelines(i.e, processors) remain active. And we can only add a new pipeline, but can't remove the existing one? And once we add multi-processor support, TracerContext would actually maintain a unique_ptr to (say) CompositeSpanProcessor which further manages all the active pipelines/processors?

Yeah. i'm adopting the design I've seen elsewhere where you can "append a pipeline", but can't remove an existing pipeline.

I.e. once a pipeline is active, it's forever active. We don't have a register/unregister mechanism.

pyohannes · 2021-04-09T23:22:02Z

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

+ const opentelemetry::sdk::resource::Resource &GetResource() const noexcept;
+
+ /**
+ * Force all active SpanProcessors to flush any buffered spans


I'm a bit confused by all active span processor. Wouldn't we just have a CompositeSpanProcessor? Which would encapsulate flushing and shutdown? So that this is a thing the CompositeSpanProcessor has to worry about, and not the TraceContext?

Personally, I wouldn't mind leaving ForceFlush and Shutdown out. GetActiveProcessor().ForceFlush() isn't that bad.

This is for users of TracerContext to understand what it does. Yes the implementation of this behavior may be implemented by "CompositeSpanProcessor" but it's not something that would be directly exposed, instead users just see "RegisterPipeline" and "Shutdown" and "ActiveProcessor". I.e. we still need to document the behavior of this class and how to use its methods.

Note: I'm not tied to "active" processor, I borrowed the name from the Java SDK.

pyohannes · 2021-04-09T23:23:24Z

sdk/include/opentelemetry/sdk/trace/tracer_provider.h

- * @return The sampler for this tracer provider.
- */
- std::shared_ptr<Sampler> GetSampler() const noexcept;
+ void RegisterProcessor(std::unique_ptr<SpanProcessor> processor) noexcept;


We have RegisterProcessor here and RegisterPipeline on the TraceContext. Could we unify the terminology here? I don't have a strong preference as to which term to use.

pyohannes · 2021-04-09T23:27:31Z

sdk/src/trace/span.cc

@@ -58,18 +58,15 @@ trace_api::SpanId GenerateRandomSpanId()
 }

 Span::Span(std::shared_ptr<Tracer> &&tracer,
- std::shared_ptr<SpanProcessor> processor,


There is a reason why the processor was put onto the span: this is to guarantee that the span/recordable gets ended on the same processor it was created with.

Otherwise, this could happen:

A span/recordable a is created from processor/exporter A.

The processor/exporter is changed, maybe replaced by a composite processor/exporter B.

When the span/recordable a is ended it is passed to the processor/exporter B and cannot be handled in this context (as it was created from processor/exporter A).

We should decide if that's still a use case we want to support. Just putting it out there that this was considered in the initial design.

Ah, I had a different proposal where a processor asks for its recordable back later and receives a nullptr (or other "false" flag).

I can revert this for the other design. I think this is a use case that needs support, there's just more than one option for how to do it.

I don't mind which option we take. I'm in favor though of guaranteeing that the Recordable passed to the exporter is of the correct type.

lalitb · 2021-04-14T19:47:55Z

@jsuereth - Would like to merge this PR once you and @pyohannes think it's good to go , that will allow me to raise PR for #664 on top of these changes :)

jsuereth · 2021-04-14T20:57:54Z

@lalitb Understood. So, to make progress, it'd be nice to know which of @pyohannes comments are blocking and which to address now.

Active Processor terminology. I don't see a counter proposal and i haven't thought of another term. I'd vote to leave it similar to Java unless there's an alternative.
Keeping SpanPrcoessor on Span vs. relying on "active span processor" in tracer. I'm ok going either way here. my preference is what I've done in this PR but I'm happy to change it.
I can update to consistently use RegisterPipeline everywhere as the spec uses the term 'pipeline" to denote processor+exporter pairing.

WDYT @pyohannes ?

pyohannes · 2021-04-14T23:47:03Z

Please let's use RegisterPipeline consistently wherever appropriate.

Otherwise I'm fine with the current approach. I was a bit confused by the missing RegisterPipeline implementation, but I think my main concerns will be handled by a proper implementation of this method and aggregate processors.

pyohannes

Thanks for getting this on the way.

ThomsonTan

Update change log?

ThomsonTan · 2021-04-15T00:08:23Z

ext/test/zpages/tracez_processor_test.cc

@@ -178,8 +178,12 @@ class TracezProcessor : public ::testing::Test
 shared_data = std::shared_ptr<TracezSharedData>(new TracezSharedData());
 processor = std::shared_ptr<TracezSpanProcessor>(new TracezSpanProcessor(shared_data));
 auto resource = opentelemetry::sdk::resource::Resource::Create({});
+ // Note: we make a *different* processor for the tracercontext. THis is because


Suggested change

// Note: we make a *different* processor for the tracercontext. THis is because

// Note: we make a *different* processor for the tracercontext. This is because

ThomsonTan · 2021-04-15T00:10:49Z

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

+ * Attaches a span processor to this tracer context.
+ *
+ * @param processor The new span processor for this tracer. This must not be
+ * a nullptr. Ownership is given to the `TracerContext`.


Suggested change

* a nullptr. Ownership is given to the `TracerContext`.

* a nullptr. Ownership is given to the `TracerContext`.

ThomsonTan · 2021-04-15T00:26:30Z

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

+ * Obtain the (conceptual) active processor.
+ *
+ * Note: When more than one processor is active, this will
+ * return an "aggregate" processor
+ */
+ SpanProcessor &GetActiveProcessor() const noexcept;


Perhaps just name it GetProcessor to avoid the confusion.

lalitb · 2021-04-15T03:51:02Z

@lalitb Understood. So, to make progress, it'd be nice to know which of @pyohannes comments are blocking and which to address now.

Active Processor terminology. I don't see a counter proposal and i haven't thought of another term. I'd vote to leave it similar to Java unless there's an alternative.

Keeping SpanPrcoessor on Span vs. relying on "active span processor" in tracer. I'm ok going either way here. my preference is what I've done in this PR but I'm happy to change it.

I can update to consistently use RegisterPipeline everywhere as the spec uses the term 'pipeline" to denote processor+exporter pairing.

WDYT @pyohannes ?

Thanks for summarising @jsuereth. don't see these as blockers - we can merge the PR once the formatting issue is fixed as that is blocking merge :)

…elemetry-cpp into wip-trace-context

jsuereth added 5 commits April 5, 2021 15:56

Migrate processor + sampler to TracerContext to ensure changes in Tra…

cdcd374

…cerProvider reflect to all tracers.

Fix build for non-SDK directories.

7a65e53

Fix cmake.

0e69d1d

Merge remote-tracking branch 'origin/main' into wip-trace-context

140519e

Run formatter.

7849f77

jsuereth marked this pull request as ready for review April 5, 2021 20:27

jsuereth requested a review from a team April 5, 2021 20:27

jsuereth added 2 commits April 5, 2021 18:38

Fixes from latest clang format.

664299e

Add bazel build for http example and fix compilation.

0dde5c4

lalitb approved these changes Apr 6, 2021

View reviewed changes

seemk approved these changes Apr 7, 2021

View reviewed changes

sdk/include/opentelemetry/sdk/trace/tracer_context.h Outdated Show resolved Hide resolved

sdk/include/opentelemetry/sdk/trace/tracer_context.h Outdated Show resolved Hide resolved

jsuereth and others added 6 commits April 7, 2021 11:19

Fixes from review and gcc 4.8

1e746e2

Fix typo.

d0ea296

Fix bazel build for http-client windows.

576cf7c

Take 2 at fixing the builds.

cb5dfdb

Another attempt to get windows to build (define curl things prior to …

2620a96

…building client lib.

Fix windows build

9d506f1

Migrate needed windows staitc libs to the curl client library and do minor cleanup to builds.

pyohannes reviewed Apr 9, 2021

View reviewed changes

pyohannes approved these changes Apr 14, 2021

View reviewed changes

ThomsonTan approved these changes Apr 15, 2021

View reviewed changes

Merge branch 'main' into wip-trace-context

ad066d9

jsuereth added 3 commits April 16, 2021 10:12

Fixes from review.

6bc5f2a

Merge remote-tracking branch 'origin/main' into wip-trace-context

3776397

Merge branch 'wip-trace-context' of https://github.com/jsuereth/opent…

1e63f71

…elemetry-cpp into wip-trace-context

jsuereth added 4 commits April 16, 2021 10:16

Ran local code format.

5033943

Manually fix style from remote tool.

01bc7c7

Last manual style fix.

ec4c83d

one last fix...

a0999fc

lalitb merged commit 7b07013 into open-telemetry:main Apr 16, 2021

jsuereth deleted the wip-trace-context branch April 19, 2021 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create shared context for updating span pipeline from TracerProvider and affecting Tracer. #650

Create shared context for updating span pipeline from TracerProvider and affecting Tracer. #650

jsuereth commented Apr 5, 2021 •

edited

Loading

codecov bot commented Apr 5, 2021 •

edited

Loading

lalitb left a comment

lalitb Apr 6, 2021

lalitb Apr 6, 2021

jsuereth Apr 6, 2021

seemk left a comment

lalitb commented Apr 7, 2021

pyohannes left a comment

pyohannes Apr 9, 2021

lalitb Apr 13, 2021

jsuereth Apr 13, 2021

ThomsonTan Apr 15, 2021

pyohannes Apr 9, 2021

jsuereth Apr 11, 2021

pyohannes Apr 12, 2021

lalitb Apr 13, 2021

jsuereth Apr 13, 2021

pyohannes Apr 9, 2021

pyohannes Apr 9, 2021

jsuereth Apr 12, 2021

pyohannes Apr 9, 2021

pyohannes Apr 9, 2021

jsuereth Apr 11, 2021

pyohannes Apr 12, 2021

lalitb commented Apr 14, 2021

jsuereth commented Apr 14, 2021

pyohannes commented Apr 14, 2021

pyohannes left a comment

ThomsonTan left a comment

ThomsonTan Apr 15, 2021

ThomsonTan Apr 15, 2021

ThomsonTan Apr 15, 2021

lalitb commented Apr 15, 2021 •

edited

Loading

	// Note: we make a different processor for the tracercontext. THis is because
	// Note: we make a different processor for the tracercontext. This is because

	* a nullptr. Ownership is given to the `TracerContext`.
	* a nullptr. Ownership is given to the `TracerContext`.

Create shared context for updating span pipeline from TracerProvider and affecting Tracer. #650

Create shared context for updating span pipeline from TracerProvider and affecting Tracer. #650

Conversation

jsuereth commented Apr 5, 2021 • edited Loading

codecov bot commented Apr 5, 2021 • edited Loading

Codecov Report

lalitb left a 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

seemk left a comment

Choose a reason for hiding this comment

lalitb commented Apr 7, 2021

pyohannes left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb commented Apr 14, 2021

jsuereth commented Apr 14, 2021

pyohannes commented Apr 14, 2021

pyohannes left a comment

Choose a reason for hiding this comment

ThomsonTan left a 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

lalitb commented Apr 15, 2021 • edited Loading

jsuereth commented Apr 5, 2021 •

edited

Loading

codecov bot commented Apr 5, 2021 •

edited

Loading

lalitb commented Apr 15, 2021 •

edited

Loading