Skip to content
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

Support for multiple processors #692

Merged
merged 24 commits into from
Apr 29, 2021
Merged

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Apr 22, 2021

Fixes #664

Changes

  • MultiSpanProcessor - Implements SpanProcessor and internally stores configured SpanProcessors.
  • MultiRecordable - implements the Recordable interface, and encapsulates several regular recordables.
  • TracerContext internally maintains the MultiSpanProcessor instance.
  • TracerContext constructor accepts vector of SpanProcessors to be configured and provide methods to add new SpanProcessors. SpanProcessor once configured cannot be deleted.

Example:
(Using TracerProvider ):

  auto exporter1 = std::unique_ptr<sdktrace::SpanExporter>(new OStreamSpanExporter);
  auto processor1 = std::unique_ptr<sdktrace::SpanProcessor>(new sdktrace::SimpleSpanProcessor(std::move(exporter1)));

  auto exporter2 = std::unique_ptr<sdktrace::SpanExporter>(new InMemorySpanExporter());
  auto processor2 = std::unique_ptr<sdktrace::SpanProcessor>(new sdktrace::SimpleSpanProcessor(std::move(exporter2)));

  auto provider = nostd::shared_ptr<sdk::trace::TracerProvider>(new sdktrace::TracerProvider(std::move(processor1));
  provider->AddProcessor(std::move(processor2));
  // Set the global trace provider
  opentelemetry::trace::Provider::SetTracerProvider(std::move(provider));

(Using TracerContext ):

  auto exporter1 = std::unique_ptr<sdktrace::SpanExporter>(new OStreamSpanExporter);
  auto processor1 = std::unique_ptr<sdktrace::SpanProcessor>(new sdktrace::SimpleSpanProcessor(std::move(exporter1)));

  auto exporter2 = std::unique_ptr<sdktrace::SpanExporter>(new InMemorySpanExporter());
  auto processor2 = std::unique_ptr<sdktrace::SpanProcessor>(new sdktrace::SimpleSpanProcessor(std::move(exporter2)));

  std::vector<std::unique_ptr<sdktrace::SpanProcessor>> processors;
  processors.push_back(std::move(processor1));
  processors.push_back(std::move(processor2));
  auto context = std::make_shared<sdktrace::TracerContext>(std::move(processors));
  auto provider = nostd::shared_ptr<opentelemetry::trace::TracerProvider>(new sdktrace::TracerProvider(context));

  // Set the global trace provider
  opentelemetry::trace::Provider::SetTracerProvider(provider);

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

@lalitb lalitb changed the title Multi processor 1 Support for multiple processors Apr 22, 2021
@lalitb lalitb changed the title Support for multiple processors [WIP] Support for multiple processors Apr 22, 2021
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #692 (74fbead) into main (a3755f4) will decrease coverage by 0.08%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
- Coverage   94.81%   94.72%   -0.09%     
==========================================
  Files         214      216       +2     
  Lines        9750     9895     +145     
==========================================
+ Hits         9244     9373     +129     
- Misses        506      522      +16     
Impacted Files Coverage Δ
sdk/src/trace/tracer_provider.cc 70.58% <41.66%> (-14.60%) ⬇️
sdk/src/trace/tracer_context.cc 72.72% <50.00%> (-7.28%) ⬇️
...ude/opentelemetry/sdk/trace/multi_span_processor.h 93.33% <93.33%> (ø)
...include/opentelemetry/sdk/trace/multi_recordable.h 93.61% <93.61%> (ø)
ext/test/zpages/tracez_data_aggregator_test.cc 96.39% <100.00%> (+0.03%) ⬆️
ext/test/zpages/tracez_processor_test.cc 98.72% <100.00%> (+0.01%) ⬆️
sdk/include/opentelemetry/sdk/trace/tracer.h 100.00% <100.00%> (ø)
sdk/src/trace/span.cc 89.01% <100.00%> (ø)
sdk/test/trace/tracer_provider_test.cc 100.00% <100.00%> (ø)
sdk/test/trace/tracer_test.cc 100.00% <100.00%> (ø)
... and 3 more

@lalitb lalitb marked this pull request as ready for review April 23, 2021 17:15
@lalitb lalitb requested a review from a team April 23, 2021 17:15
@lalitb lalitb changed the title [WIP] Support for multiple processors Support for multiple processors Apr 23, 2021
}
}

void AddProcessor(std::unique_ptr<SpanProcessor> &&processor)
Copy link
Member

@reyang reyang Apr 23, 2021

Choose a reason for hiding this comment

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

If the intention is to allow processors to be added at runtime, we will need to cover thread safety.

For example, this is thread-safe in a single-writer-multi-reader situation (e.g. the processors chain is handling spans from multiple threads, while one thread is trying to add a new processor).

Given here we're using STL vector, I guess we'll run into race condition - if one thread is trying to add a processor while other threads are sending spans to the processor chain, we might get broken state.

Copy link
Member Author

@lalitb lalitb Apr 23, 2021

Choose a reason for hiding this comment

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

Good comment. My earlier code did have mutex guard against all vector operations in this class, somehow missed out to checkin that. The doubly linked list (as done in dotnet) looks like a good option, would be good to do some benchmarking to select the optimal one of two.

Copy link
Member

@reyang reyang Apr 23, 2021

Choose a reason for hiding this comment

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

I can share the thinking from the .NET implementation:

STL vector + mutex would give the ultimate thread-safety in multi-reader-multi-writer situation, the potential downside is that it will introduce contention to the readers (in this case, the readers are the threads that push spans to the processor chain), which might be something to avoid.

Doubly linked list only gives single-writer-multi-reader thread-safety, however it is well aligned with the general .NET API guideline (that if a class method doesn't specify thread safety, by default it is not supposed to be called concurrently). And if we need the ultimate thread safety later, we can introduce a lock object which is shared among writers (but not readers), in this way we have the minimum contention on the hot path.

Copy link
Member Author

@lalitb lalitb Apr 23, 2021

Choose a reason for hiding this comment

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

Ok. now I realize why I removed the mutex guard - as MultiSpanProcessor is an atomic instance in TracerContext :

opentelemetry::sdk::common::AtomicUniquePtr<SpanProcessor> processor_;

And so multiple threads shouldn't be able to access it simultaneously.

Copy link
Contributor

Choose a reason for hiding this comment

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

Atomic just means that you can compare-and-swap the pointer. Users can still access it simultaneously (after reading the pointer in memory).

if you have AddProcessor return a new MultiProcessor instance and compare-and-swap the Atomic Ptr then I think you'd be ok, but as is, you still have a (possible) threading issue if more than thread calls AddProcessor. It's unlikely but still possible.

Copy link
Member Author

@lalitb lalitb Apr 29, 2021

Choose a reason for hiding this comment

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

OK, I think I got your point, in that case, we should have Atomic Ptr for Span Processor, and create new instance of MultiProcessor everytime AddProcessor() is called. It is already documented that AddProcessor() is not thread safe, and shouldn't be called simultaneously. I will do these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed that AddProcessor was marked not thread-safe as well. This means my point is not important.

If the changes to make AddProcessor thread-safe aren't too painful, I think it's worth it. I think it's a low-frequency method call, so it's ok to be "expensive" to AddProcessor as long as the "read" hot path is fast (which you already have AFAICT). Removing my requested changes. I'd prefer to see AddProcessor thread-safe but this is fine as-is.

Co-authored-by: Reiley Yang <reyang@microsoft.com>
@@ -0,0 +1,12 @@

# Simple Trace Example
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty first line and change the title to indicate this is for multiple processors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, have changed the title, and the description too.

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Method name improvements LGTM for Processor / AddProcessor.

+1 to Reiley's comments on thread safety, will re-review when that's updated, but like this approach!

// Default is an always-on sampler.
auto context = std::make_shared<sdktrace::TracerContext>(std::move(processor));
auto context = std::make_shared<sdktrace::TracerContext>(std::move(processors));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a convenience constructor that just takes on processor?

Alternatively, a static helper method to make TracerProvider from a processor?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Consider taking a look at the other language clients that reached 1.0 stable and see what's the best practice.
One more example from Python https://opentelemetry-python.readthedocs.io/en/latest/getting-started.html.

Copy link
Member Author

@lalitb lalitb Apr 24, 2021

Choose a reason for hiding this comment

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

This specific example is using TracerContext. This is how it looks like if we directly use TracerProvider to add processors:
( which would be what application developer be doing most of the times ):

  auto provider = nostd::shared_ptr<opentelemetry::trace::TracerProvider>(std:move(processor1), resource, sampler, id_generator);
  provider.AddProcessor(std::move(processor2));
  provider.AddProcessor(std::move(processor3));

  opentelemetry::trace::Provider::SetTracerProvider(provider);

Do we want this for TracerContext too ?

Copy link
Member Author

@lalitb lalitb Apr 26, 2021

Choose a reason for hiding this comment

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

I have modified the example NOT to use TracerContext, instead directly use TracerProvider to pass processor(s) as an argument to constructor. TracerProvider constructor takes either of single processor, or vector of processors as an argument as shown in example in above comment ( and in PR description ).

@lalitb
Copy link
Member Author

lalitb commented Apr 26, 2021

Method name improvements LGTM for Processor / AddProcessor.

+1 to Reiley's comments on thread safety, will re-review when that's updated, but like this approach!

As suggested by @reyang , the following changes are done

  • Modify sdk::trace::MultiSpanProcessor class to store processors in a list instead of vector.
  • Remove atomicity from TracerContext::processor_ ( i.e change it's type from opentelemetry::sdk::common::AtomicUniquePtr<SpanProcessor> to std::unique_ptr<SpanProcessor>. This is because the list structure in MultiSpanProcessor will take care of single write, multiple read scenario for processors.
  • TracerProvider::AddProcessor() and TracerContext::AddProcessor() methods are no longer thread safe, and have added that as comment for them.
  • No change but to mention: We don't support removing processors,

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

ThomsonTan
ThomsonTan previously approved these changes Apr 26, 2021
examples/multi_processor/main.cc Show resolved Hide resolved
examples/multi_processor/README.md Outdated Show resolved Hide resolved
const std::unique_ptr<Recordable> &GetRecordable(const SpanProcessor &processor) const noexcept
{
// TODO - return nullptr ref on failed lookup?
static std::unique_ptr<Recordable> empty(nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this empty could be moved to the line right above return empty? We don't need initialize this value if recordable is found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could make the empty variable as class static as it is also useful in ReleaseRecordable?

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 empty could be moved to the line right above return empty? We don't need initialize this value if recordable is found.

done

Or we could make the empty variable as class static as it is also useful in ReleaseRecordable?

That won't help as ReleaseRecordable needs to return unique_ptr, not it's a reference so class static can't be used. But control would never reach both places as we don't support removing processors once configured.

@ThomsonTan ThomsonTan dismissed their stale review April 26, 2021 18:27

Miss clicked.

@lalitb
Copy link
Member Author

lalitb commented Apr 27, 2021

Will see if there are comments from @jsuereth, @pyohannes, and others before closing it,

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

I think there's still a threading issue around AddProcessor.

You could possible solve this via fully-copying the processors array in the "AddProcessor" method of TracerContext (and then use a compare-and-swap to flip instances) meaning no threads would have an inconsistent processor. However, the code, as it stands, has a very unlikely threading issue.


const std::unique_ptr<Recordable> &GetRecordable(const SpanProcessor &processor) const noexcept
{
// TODO - return nullptr ref on failed lookup?
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove the TODO

}
}

void AddProcessor(std::unique_ptr<SpanProcessor> &&processor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Atomic just means that you can compare-and-swap the pointer. Users can still access it simultaneously (after reading the pointer in memory).

if you have AddProcessor return a new MultiProcessor instance and compare-and-swap the Atomic Ptr then I think you'd be ok, but as is, you still have a (possible) threading issue if more than thread calls AddProcessor. It's unlikely but still possible.

@lalitb
Copy link
Member Author

lalitb commented Apr 28, 2021

I think there's still a threading issue around AddProcessor.

You could possible solve this via fully-copying the processors array in the "AddProcessor" method of TracerContext (and then use a compare-and-swap to flip instances) meaning no threads would have an inconsistent processor. However, the code, as it stands, has a very unlikely threading issue.

Thank you @jsuereth for the comments. Just to have a better understanding of the issue - the current approach of switching to the linked-list structure still has race condition around AddProcessor, and another approach would be moving back to vector, and do full-copying followed by flip?

}
}

void AddProcessor(std::unique_ptr<SpanProcessor> &&processor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed that AddProcessor was marked not thread-safe as well. This means my point is not important.

If the changes to make AddProcessor thread-safe aren't too painful, I think it's worth it. I think it's a low-frequency method call, so it's ok to be "expensive" to AddProcessor as long as the "read" hot path is fast (which you already have AFAICT). Removing my requested changes. I'd prefer to see AddProcessor thread-safe but this is fine as-is.

@jsuereth
Copy link
Contributor

@lalitb

Thank you @jsuereth for the comments. Just to have a better understanding of the issue - the current approach of switching to the linked-list structure still has race condition around AddProcessor, and another approach would be moving back to vector, and do full-copying followed by flip?

Yeah, I think you have two options:

  1. In AddProcessor, just do a big compare-and-swap of the whole (immutable-ish) Processor. This should have the advantage that reads are fast, writes are consistent and it should be less work overall.
  2. The double-linked list approach could work, but you likely need to do some memory-barriers / compare-and-swap routines on top of the code you already have to ensure no one views the linked list in an inconsistent state (which, having two pointers, means there's a slight chance).

@reyang
Copy link
Member

reyang commented Apr 29, 2021

Ah, I missed that AddProcessor was marked not thread-safe as well. This means my point is not important.

If the changes to make AddProcessor thread-safe aren't too painful, I think it's worth it. I think it's a low-frequency method call, so it's ok to be "expensive" to AddProcessor as long as the "read" hot path is fast (which you already have AFAICT). Removing my requested changes. I'd prefer to see AddProcessor thread-safe but this is fine as-is.

FYI - a slightly different perspective for consideration, in C# we chose to make AddProcessor not thread safe (see my explanation here) for couple reasons:

  1. AddProcessor has side effect since processors are meant to be called in FIFO sequence. Having multiple threads adding processors simultaneously is normally an indication of application bug as the sequence is undeterministic.
  2. In C# the convention is that member functions are not thread-safe unless explicitly specified. I think C++ STL follows the same convention.

Additional thoughts:

Maybe we should clarify this in the spec.
Currently the API spec requires all functions on TracerProvider to be thread safe.
The SDK spec doesn't say anything about concurrency (and AddProcessor is exposed by SDK not the API).

@lalitb
Copy link
Member Author

lalitb commented Apr 29, 2021

  1. AddProcessor has side effect since processors are meant to be called in FIFO sequence. Having multiple threads adding processors simultaneously is normally an indication of application bug as the sequence is undeterministic.

That's a logical reason to enforce non-thread-safe behavior for AddProcessor

@lalitb
Copy link
Member Author

lalitb commented Apr 29, 2021

@reyang @jsuereth - Thanks for the comments. I am merging it in the double linked-list based approach as per the discussions here. Atm ptr is not used as AddProcessor() is not thread-safe. Will make it explicit in sdk document. Also will create a ticket to discuss and revisit its design in the future if we need to.

@lalitb lalitb merged commit a987f0a into open-telemetry:main Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple SpanProcessors per TracerProvider
4 participants