Skip to content

Adding a tracer provider - #24

Merged
reyang merged 47 commits into
open-telemetry:masterfrom
lykkin:tracer-registry
Mar 17, 2020
Merged

reyang merged 47 commits into
open-telemetry:masterfrom
lykkin:tracer-registry

Conversation

@lykkin

@lykkin lykkin commented Jan 7, 2020

Copy link
Copy Markdown
Contributor

There seems to be some movement on implementing things from spans up, so I thought I would start building from the tracer factory down.

This currently requires C++14 for its use of unique_ptrs. I know there was some talk about wanting to limit the standard to C++11, but not sure how hard this requirement is.

Comment thread api/include/opentelemetry/trace/tracer-registry.h Outdated
Comment thread api/src/opentelemetry/trace/tracer-registry.cc Outdated
@lykkin

lykkin commented Jan 7, 2020

Copy link
Copy Markdown
Contributor Author

I signed it

Comment thread api/include/opentelemetry/trace/tracer.h Outdated
Comment thread api/src/opentelemetry/trace/tracer-registry.cc Outdated
Comment thread api/include/opentelemetry/trace/tracer-registry.h Outdated
Comment thread api/include/opentelemetry/trace/tracer-registry.h Outdated
@reyang

reyang commented Jan 7, 2020

Copy link
Copy Markdown
Member

It'll be great to point to the specification.
As what we have today, the spec is using TracerFactory and getTracer.
Also it'll be helpful to clarify the thread-safety requirement.

Comment thread api/include/opentelemetry/trace/tracer-registry.h Outdated
@lykkin
lykkin requested review from jmacd and reyang January 7, 2020 21:42
Comment thread api/include/opentelemetry/trace/tracer.h Outdated
Comment thread api/include/opentelemetry/trace/tracer-factory.h Outdated
@SaintDubious

Copy link
Copy Markdown

How Serious are we about the "header only" requirement specified here: https://github.com/open-telemetry/opentelemetry-cpp/blob/master/docs/requirements.md

@g-easy

g-easy commented Jan 9, 2020

Copy link
Copy Markdown
Contributor

tracer-factory.h

Should this be tracer_factory.h with an underscore, to match e.g. string_view.h?

@lykkin

lykkin commented Jan 9, 2020

Copy link
Copy Markdown
Contributor Author

tracer-factory.h

Should this be tracer_factory.h with an underscore, to match e.g. string_view.h?

Good call, will change.

jmacd
jmacd previously requested changes Jan 9, 2020
Comment thread sdk/include/opentelemetry/sdk/trace/tracer_factory.h Outdated
@lykkin
lykkin force-pushed the tracer-registry branch 2 times, most recently from 4ef274d to 8393c4d Compare January 14, 2020 16:43
@jmacd

jmacd commented Jan 14, 2020

Copy link
Copy Markdown
Member

This looks good. One nit: open-telemetry/oteps#76 says that we'll standardize on the term "Provider" instead or "Factory".

@lykkin

lykkin commented Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

This looks good. One nit: open-telemetry/oteps#76 says that we'll standardize on the term "Provider" instead or "Factory".

Thanks, I'll update that.

What do you think about making the return value of GetTracerFactory a shared_ptr<TracerFactory>? Since users can construct factories outside the global scope and pass it in, there's some questionable ownership of that pointer at that point. I can either add that in as part of this PR or make a quick follow on PR after this gets merged. I'd prefer the latter, as I would need to make nostd::shared_ptr for the ABI compat restriction.

@lykkin
lykkin requested a review from jmacd January 15, 2020 19:48
Comment thread api/BUILD Outdated
Comment thread api/include/opentelemetry/global/provider.h Outdated
@pyohannes

Copy link
Copy Markdown
Contributor

@reyang: I just rebased the branch.

@reyang
reyang merged commit 752adcf into open-telemetry:master Mar 17, 2020
@pyohannes pyohannes mentioned this pull request Jun 10, 2020
0x4b pushed a commit to 0x4b/opentelemetry-cpp that referenced this pull request Aug 19, 2020
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.

7 participants