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

Exposing tracer property on spans #21

Closed
SergeyKanzhelev opened this issue May 23, 2019 · 9 comments
Closed

Exposing tracer property on spans #21

SergeyKanzhelev opened this issue May 23, 2019 · 9 comments
Assignees
Milestone

Comments

@SergeyKanzhelev
Copy link
Member

From open-telemetry/opentelemetry-js#5

Span has a private tracer property, NoRecordSpan doesn't. I'd prefer to make tracer always available on Span like in OpenTracing. Coming from OpenTracing, I found it challenging not having tracer available on spans, especially that first I assumed it is and started to use it, then my code broke one NoRecordSpan(s).

@hekike, @rochdev

@tylerbenson
Copy link
Member

I also think it would be nice since configs are usually exposed via the tracer. Besides, I assume Span will need to have a reference to it anyway to report when finished.

@SergeyKanzhelev
Copy link
Member Author

@tylerbenson in the original issue - the scenario was given to start a child span, not to distinguish configurations. How will you use tracer obtained from Span besides starting child spans?

@rochdev
Copy link
Member

rochdev commented May 25, 2019

We use this with OpenTracing to access the scope manager and to start related spans. The ability to access the tracer configuration would also be a nice addition since right now with OpenTracing the only compliant way to do that would be to assign the configuration to every span context to expose it.

In general, I find that exposing the tracer would result in a cleaner API that replicating some of the available tracer methods on the span class.

@SergeyKanzhelev SergeyKanzhelev modified the milestones: API complete, API revision: 07-2019 May 31, 2019
@carlosalberto
Copy link
Contributor

I remember that we wanted to do this in OT Java (although we did this for OT Python, for example), and I'm definitely in agreement with this change.

Wondering if there's a way to poke people for a counter example (if there's any at all ;) )

@SergeyKanzhelev
Copy link
Member Author

This becoming even more important and valuable with the named tracers

@tedsuo
Copy link
Contributor

tedsuo commented Oct 4, 2019

@SergeyKanzhelev I agree. Given the design of span.end, it's extremely unlikely that a span implementation would not to have a tracer reference, so I doubt there is a downside to this.

@Oberon00
Copy link
Member

Oberon00 commented Oct 4, 2019

In my opinion, with named tracers, there now actually is the slight downside to providing another way to access the tracer. E.g. some instrumentation libraries might use parent.getTracer().startSpan(parent) instead of the "proper" tracerFactory.getTracer("myname", "myversion").startSpan(parent). The the span will be misattributed to the instrumentation library that created the parent span.

@SergeyKanzhelev
Copy link
Member Author

@Oberon00 it is a good point that some libraries may want to "hide" their Tracer. For instance, user may disable certain component telemetry and span will carry no-op Tracer. Which will not be clear to the caller.

Giving it's easy to obtain a tracer and tracer doesn't carry any instance-specific information I tend to close this issue.

We should reconsider when Tracer will have more methods than it has today.

@yurishkuro
Copy link
Member

+1 for closing this issue without action. In OpenTracing we had Span.getTracer() method and there was a lot of confusion about it. Tracer (or factory) should only be an injected dependency, accessing it at runtime from a span is a sign of code smell.

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
* Start filling out span backport

* Fix assertion

* Add a span test.

* Fix constructors

* Fill out span tests

* Add test coverage for pointer-count construction

* s/ASSERT_DEATH/EXPECT_DEATH/

* Add bracket operator test

* Add test coverage for other span construction.

* Add test coverage for array construction

* Add test for range construction

* Fix typo.

* Add assignment test

* Add iteration test

* Add data/size functions

* Add array overloads

* Support implicit construction from general containers

* Reformat

* Fix typo

* Fix typo

* Fix enable_if condition
rockb1017 pushed a commit to rockb1017/opentelemetry-specification that referenced this issue Nov 18, 2021
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 21, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 23, 2024
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

No branches or pull requests

8 participants