-
Notifications
You must be signed in to change notification settings - Fork 423
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
Create shared context for updating span pipeline from TracerProvider and affecting Tracer. #650
Changes from 13 commits
cdcd374
7a65e53
0e69d1d
140519e
7849f77
664299e
0dde5c4
1e746e2
d0ea296
576cf7c
cb5dfdb
2620a96
9d506f1
ad066d9
6bc5f2a
3776397
1e63f71
5033943
01bc7c7
ec4c83d
a0999fc
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 |
---|---|---|
@@ -0,0 +1,42 @@ | ||
cc_binary( | ||
name = "example_http_client", | ||
srcs = [ | ||
"tracer_common.hpp", | ||
"client.cc", | ||
], | ||
deps = [ | ||
"//api", | ||
"//ext:headers", | ||
"//ext/src/http/client/curl:http_client_curl", | ||
"//exporters/ostream:ostream_span_exporter", | ||
"//sdk/src/trace", | ||
], | ||
# TODO: Move copts/linkopts for static CURL usage into shared bzl file. | ||
copts = [ | ||
"-DCURL_STATICLIB", | ||
"-DWITH_CURL", | ||
], | ||
linkopts = select({ | ||
"//bazel:windows": [ | ||
"-DEFAULTLIB:advapi32.lib", | ||
"-DEFAULTLIB:crypt32.lib", | ||
"-DEFAULTLIB:Normaliz.lib", | ||
], | ||
"//conditions:default": [], | ||
}), | ||
) | ||
|
||
cc_binary( | ||
name = "example_http_server", | ||
srcs = [ | ||
"server.cc", | ||
"tracer_common.hpp", | ||
"server.hpp", | ||
], | ||
deps = [ | ||
"//api", | ||
"//ext:headers", | ||
"//exporters/ostream:ostream_span_exporter", | ||
"//sdk/src/trace", | ||
], | ||
) | ||
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
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.
Suggested change
|
||||||
// all the tests use shared data, and we want to make sure this works correctly. | ||||||
auto context = std::make_shared<TracerContext>( | ||||||
std::unique_ptr<SpanProcessor>(new TracezSpanProcessor(shared_data)), resource); | ||||||
|
||||||
tracer = std::shared_ptr<opentelemetry::trace::Tracer>(new Tracer(processor, resource)); | ||||||
tracer = std::shared_ptr<opentelemetry::trace::Tracer>(new Tracer(context)); | ||||||
auto spans = shared_data->GetSpanSnapshot(); | ||||||
running = spans.running; | ||||||
completed = std::move(spans.completed); | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,85 @@ | ||||||
#pragma once | ||||||
|
||||||
#include "opentelemetry/sdk/common/atomic_unique_ptr.h" | ||||||
#include "opentelemetry/sdk/resource/resource.h" | ||||||
#include "opentelemetry/sdk/trace/processor.h" | ||||||
#include "opentelemetry/sdk/trace/samplers/always_on.h" | ||||||
#include "opentelemetry/version.h" | ||||||
|
||||||
OPENTELEMETRY_BEGIN_NAMESPACE | ||||||
namespace sdk | ||||||
{ | ||||||
namespace trace | ||||||
{ | ||||||
|
||||||
/** | ||||||
* A class which stores the TracerProvider context. | ||||||
* | ||||||
* This class meets the following design criteria: | ||||||
* - A shared reference between TracerProvider and Tracers instantiated. | ||||||
* - A thread-safe class that allows updating/altering processor/exporter pipelines | ||||||
* and sampling config. | ||||||
* - The owner/destroyer of Processors/Exporters. These will remain active until | ||||||
* this class is destroyed. I.e. Sampling, Exporting, flushing etc. are all ok if this | ||||||
* object is alive, and they will work together. If this object is destroyed, then | ||||||
* no shared references to Processor, Exporter, Recordable etc. should exist, and all | ||||||
* associated pipelines will have been flushed. | ||||||
*/ | ||||||
class TracerContext | ||||||
{ | ||||||
public: | ||||||
explicit TracerContext(std::unique_ptr<SpanProcessor> processor, | ||||||
opentelemetry::sdk::resource::Resource resource = | ||||||
opentelemetry::sdk::resource::Resource::Create({}), | ||||||
std::unique_ptr<Sampler> sampler = | ||||||
std::unique_ptr<AlwaysOnSampler>(new AlwaysOnSampler)) noexcept; | ||||||
/** | ||||||
* 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`. | ||||||
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.
Suggested change
|
||||||
*/ | ||||||
void RegisterPipeline(std::unique_ptr<SpanProcessor> processor) noexcept; | ||||||
|
||||||
/** | ||||||
* Obtain the sampler associated with this tracer. | ||||||
* @return The sampler for this tracer. | ||||||
*/ | ||||||
Sampler &GetSampler() const noexcept; | ||||||
|
||||||
/** | ||||||
* Obtain the (conceptual) active processor. | ||||||
* | ||||||
* Note: When more than one processor is active, this will | ||||||
* return an "aggregate" processor | ||||||
*/ | ||||||
SpanProcessor &GetActiveProcessor() const noexcept; | ||||||
Comment on lines
+51
to
+56
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. What would be an inactive processor? 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 don't think there is any 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. 'inactive' would be any constructed but not registered processor. I'm not tied to this name, just took it from Java ;) 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. Perhaps just name it 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. We might theoretically run into issue here when some tracer does some work with a processor obtained by 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. 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. 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 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? 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. 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, 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. 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. |
||||||
|
||||||
/** | ||||||
* Obtain the resource associated with this tracer context. | ||||||
* @return The resource for this tracer context. | ||||||
*/ | ||||||
const opentelemetry::sdk::resource::Resource &GetResource() const noexcept; | ||||||
|
||||||
/** | ||||||
* Force all active SpanProcessors to flush any buffered spans | ||||||
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'm a bit confused by all active span processor. Wouldn't we just have a 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. Personally, I wouldn't mind leaving 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 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. |
||||||
* within the given timeout. | ||||||
*/ | ||||||
bool ForceFlush( | ||||||
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept; | ||||||
|
||||||
/** | ||||||
* Shutdown the span processor associated with this tracer provider. | ||||||
*/ | ||||||
bool Shutdown() noexcept; | ||||||
|
||||||
private: | ||||||
// This is an atomic pointer so we can adapt the processor pipeline dynamically. | ||||||
opentelemetry::sdk::common::AtomicUniquePtr<SpanProcessor> processor_; | ||||||
opentelemetry::sdk::resource::Resource resource_; | ||||||
std::unique_ptr<Sampler> sampler_; | ||||||
}; | ||||||
|
||||||
} // 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.
Thanks for adding the bazel build for this.