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

Separate interfaces from no-op impls #166

Closed
c24t opened this issue Sep 24, 2019 · 7 comments
Closed

Separate interfaces from no-op impls #166

c24t opened this issue Sep 24, 2019 · 7 comments
Labels
api Affects the API package. discussion Issue or PR that needs/is extended discussion. metrics tracing
Milestone

Comments

@c24t
Copy link
Member

c24t commented Sep 24, 2019

From #160 (comment), related to #165.

We don't currently distinguish between abstract classes/interfaces and no-op implementations in the API package. We expect implementations to override the no-op opentelemetry.trace.Tracer class, which also serves as the Tracer interface. This pattern causes problems for Meter in #160 because we don't have a no-op implementation.

Consider separating these classes, and making it impossible to accidentally load interfaces by making them ABCs, as in Resource.

@c24t c24t added api Affects the API package. tracing metrics labels Sep 24, 2019
@Oberon00
Copy link
Member

because we don't have a no-op implementation.

That's the problem then, we need a no-op implementation for the Meter. I don't see how separating ABCs and no-ops helps.

Relatedly, the tracer no-op mode is also in a bad shape: #142.

@c24t
Copy link
Member Author

c24t commented Sep 27, 2019

I don't see how separating ABCs and no-ops helps.

This is assuming we don't actually want implementers extending the no-op classes, but there may be an argument for writing them to be extended instead. As it is now, a Tracer or Meter with some non-overridden no-op methods wouldn't be very useful, and one of the benefits of ABC classes is that makes it explicit which methods need to be overridden.

@c24t c24t added the discussion Issue or PR that needs/is extended discussion. label Oct 11, 2019
@c24t c24t added this to the Alpha v0.3 milestone Oct 11, 2019
@codeboten
Copy link
Contributor

Would the idea here be to define a no-op tracer and to make Meter, Span and Tracer ABCs?

@c24t
Copy link
Member Author

c24t commented Dec 3, 2019

Would the idea here be to define a no-op tracer and to make Meter, Span and Tracer ABCs?

Yes! With the goal of clearly separating the API and default implementation.

@toumorokoshi
Copy link
Member

I think @Oberon00 raised a really good point that Vendors will most likely extend the default implementations, as it provides a layer of safety where newly required methods will not cause the SDK they build to fail.

As ABCs only provide value if they're used directly, we might as well make Default / No-op interfaces and make that the pattern.

@Oberon00
Copy link
Member

Oberon00 commented Dec 6, 2019

BTW, it seems this issue is actually a duplicate of #66.

@c24t
Copy link
Member Author

c24t commented Dec 16, 2019

Closing this one in favor of #66.

@c24t c24t closed this as completed Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. discussion Issue or PR that needs/is extended discussion. metrics tracing
Projects
None yet
Development

No branches or pull requests

4 participants