Separate no-op from interfaces #311

codeboten · 2019-11-28T22:09:43Z

Fixes #66

Started on this today. Providing a no-op for Meter is easy. Separating the Tracer interface from a no-op implementation is a bit more tricky. It requires changes to the loader as the following code tries to load an implementation of a Tracer, and if it fails it tries to load the Tracer interface itself which won't work with an ABC:

opentelemetry-python/opentelemetry-api/src/opentelemetry/util/loader.py

Lines 162 to 172 in d3bb228

    
           def _load_impl( 
        
               api_type: Type[_T], factory: Optional[Callable[[Type[_T]], Optional[_T]]] 
        
           ) -> _T: 
        
               """Tries to load a configured implementation, if unsuccessful, returns a 
        
               fast no-op implemenation that is always available. 
        
               """ 
        
               result = _try_load_configured_impl(api_type, factory) 
        
               if result is None: 
        
                   return api_type() 
        
               return result

Signed-off-by: Alex Boten [email protected]

Signed-off-by: Alex Boten <[email protected]>

codecov-io · 2019-11-28T22:20:22Z

Codecov Report

Merging #311 into master will decrease coverage by 0.84%.
The diff coverage is 79.5%.

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
- Coverage   85.76%   84.91%   -0.85%     
==========================================
  Files          33       38       +5     
  Lines        1672     1883     +211     
  Branches      182      217      +35     
==========================================
+ Hits         1434     1599     +165     
- Misses        190      219      +29     
- Partials       48       65      +17

Impacted Files	Coverage Δ
opentelemetry-api/src/opentelemetry/util/loader.py	`81.25% <ø> (ø)`	⬆️
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py	`99.05% <ø> (ø)`	⬆️
...ntelemetry-api/src/opentelemetry/trace/sampling.py	`85.1% <ø> (ø)`	⬆️
...sdk/src/opentelemetry/sdk/trace/export/__init__.py	`86.79% <0%> (ø)`	⬆️
...try-ext-wsgi/src/opentelemetry/ext/wsgi/version.py	`100% <100%> (ø)`	⬆️
...pentelemetry-sdk/src/opentelemetry/sdk/__init__.py	`100% <100%> (ø)`	⬆️
...-ext-flask/src/opentelemetry/ext/flask/__init__.py	`88.88% <100%> (ø)`
...elemetry-api/src/opentelemetry/metrics/__init__.py	`87.93% <100%> (+1.96%)`	⬆️
.../src/opentelemetry/ext/opentracing_shim/version.py	`100% <100%> (ø)`
...app/src/opentelemetry_example_app/flask_example.py	`100% <100%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 607ff4b...bcaaf6b. Read the comment docs.

Signed-off-by: Alex Boten <[email protected]>

codeboten · 2019-12-03T15:56:35Z

Updated Tracer and Span to be ABCs as well. Added a try block to the loader to default to the DefaultTracer when any other implementation fails to load https://github.com/open-telemetry/opentelemetry-python/pull/311/files#diff-97345450e765fa40d32664356325e06cR601-R606

c24t

Changes LGTM, and I think this is the right approach for the APIs in general. I have a few questions about default values.

opentelemetry-api/src/opentelemetry/trace/__init__.py

c24t · 2019-12-03T23:15:23Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

+        # pylint: disable=unused-argument,no-self-use
+        return INVALID_SPAN
+
+    @contextmanager  # type: ignore


What's the right solution for context manager types? I see we've been adding # type: ignore to these since (at least) #92, but couldn't find a discussion about this.

This is tracked in #219. The best solution I know is still #198 (comment).

c24t · 2019-12-03T23:23:39Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -525,7 +598,12 @@ def tracer() -> Tracer:

    if _TRACER is None:
        # pylint:disable=protected-access
-        _TRACER = loader._load_impl(Tracer, _TRACER_FACTORY)
+        try:
+            _TRACER = loader._load_impl(Tracer, _TRACER_FACTORY)  # type: ignore


Suggested change

_TRACER = loader._load_impl(Tracer, _TRACER_FACTORY) # type: ignore

_TRACER = loader._load_impl(DefaultTracer, _TRACER_FACTORY) # type: ignore

Would this do the same thing as the try/catch?

Using a DefaultTracer would mean any implementation would be extending DefaultTracer rather than the Tracer interface, which means we would lose the value of using abstract methods to signal what methods are expected to be implemented.

I think this try: catch should really be moved to the loader. Probably some reflection will be required (or a class attribute DEFAULT_IMPLEMENTATION_CLASS linking the two (alternatively INTERFACE_CLASS in the other direction)

But if I were a vendor, I would definitely inherit from DefaultTracer instead of Tracer because I don't want my code to crash the customer's app if they start calling a new method that my Tracer doesn't (yet) implement. Which was also one argument by me (and I think @carlosalberto agreed) against this ABC/Default split in #66 (#66 (comment) to be exact).

I think that's a really good argument that almost convinces me against ABCs. I'm +1 to doing that work in this PR, or going back to switching to no-ops once we have all of this standardized.

Yeah, definitely same feeling about vendors ending up inheriting from DefaultTracer.

c24t · 2019-12-03T23:32:39Z

opentelemetry-api/tests/test_loader.py

@@ -49,7 +49,7 @@ def setUp(self):

    def test_get_default(self):
        tracer = trace.tracer()
-        self.assertIs(type(tracer), trace.Tracer)
+        self.assertIs(type(tracer), trace.DefaultTracer)


@Oberon00 might want to check that this is testing the right thing still. It seems like DummyTracer should still extend Tracer here, but that seems to rely on instantiating Tracers.

Alternatively here I can implement the abstract methods in DummyTracer and that should work as well

I think implementing the abstract methods would be worth it here.

Co-Authored-By: Chris Kleinknecht <[email protected]>

Signed-off-by: Alex Boten <[email protected]>

Fixes open-telemetry#245

… into issue_166

c24t

LGTM, let's see if @Oberon00 has any comments.

Oberon00

I have no strong opinion (I weakly prefer not splitting ABC/Default but I won't have time to argue in the near future)

toumorokoshi

Thanks for the work here! Standardizing our policy would be great.

toumorokoshi · 2019-12-06T18:30:35Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -525,7 +598,12 @@ def tracer() -> Tracer:

    if _TRACER is None:
        # pylint:disable=protected-access
-        _TRACER = loader._load_impl(Tracer, _TRACER_FACTORY)
+        try:
+            _TRACER = loader._load_impl(Tracer, _TRACER_FACTORY)  # type: ignore


I think that's a really good argument that almost convinces me against ABCs. I'm +1 to doing that work in this PR, or going back to switching to no-ops once we have all of this standardized.

Signed-off-by: Alex Boten <[email protected]>

Oberon00 · 2019-12-09T09:07:29Z

Note that in #66 we decided that we do not want to use ABCs. It seems that we have revised our decision here and in #166, but I want to make sure that everyone is aware of that. Please make sure you understand the reasons for the decision from #66 and that you still want to do this.

I'm removing the "please merge" label again for now (ofc you are still free to merge but it isn't something that should be merged without consideration).

lzchen · 2019-12-09T19:41:32Z

opentelemetry-api/src/opentelemetry/metrics/__init__.py

+        # pylint: disable=no-self-use
+        return DefaultMetric()
+
+


I think get_label_set will be needed too.

Fixed, i'd forgotten to make get_label_set abstract.

Updated TracerSource to also use ABC, created the DefaultTracerSource no-op implementation. Signed-off-by: Alex Boten <[email protected]>

c24t

One question about abstract method implementations, and it looks like we still need an abstract get_label_set, but otherwise it LGTM!

c24t · 2020-01-09T19:48:27Z

opentelemetry-api/src/opentelemetry/metrics/__init__.py

@@ -211,6 +212,7 @@ def record_batch(
                corresponding value to record for that metric.
        """

+    @abc.abstractmethod


What are your thoughts on abstract methods having implementations? I see how it could be useful to have a safe default implementation that the user has to explicitly call with super, but I see that DefaultMeter has its own implementation.

For now, I'll remove the default implementation from the interfaces. I don't have a strong preference either ways, but it's less changes to remove the few that were left behind than to add an implementation for all abstract methods.

c24t · 2020-01-09T20:09:00Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -175,12 +178,14 @@ def get_context(self) -> "SpanContext":
        # pylint: disable=no-self-use
        return INVALID_SPAN_CONTEXT


Same question about implementation here.

c24t · 2020-01-09T20:11:57Z

opentelemetry-api/src/opentelemetry/trace/__init__.py



-class Tracer:
+class DefaultTracerSource(TracerSource):
+    """The default TracerSource, used when no TracerSource implementation is available.


Suggested change

"""The default TracerSource, used when no TracerSource implementation is available.

"""The default TracerSource, used when no implementation is available.

or

"""The default TracerSource, used when no implementation is available.

opentelemetry-api/tests/test_implementation.py

Signed-off-by: Alex Boten <[email protected]>

closes open-telemetry#311 Signed-off-by: Olivier Albertini <[email protected]>

make Meter an ABC

4a4ef9e

Signed-off-by: Alex Boten <[email protected]>

codeboten added the WIP Work in progress label Nov 28, 2019

codeboten changed the title ~~make Meter an ABC~~ Separate no-op from interfaces Nov 28, 2019

Alex Boten added 4 commits December 2, 2019 15:30

making Span an ABC

1696372

Signed-off-by: Alex Boten <[email protected]>

Merge remote-tracking branch 'origin/master' into issue_166

e3ae9c1

updating DefaultMeter to support LabelSet

8df0d72

Signed-off-by: Alex Boten <[email protected]>

make Tracer an ABC

ae2b960

Signed-off-by: Alex Boten <[email protected]>

codeboten marked this pull request as ready for review December 3, 2019 15:56

codeboten requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners December 3, 2019 15:56

codeboten removed the WIP Work in progress label Dec 3, 2019

c24t reviewed Dec 3, 2019

View reviewed changes

codeboten and others added 4 commits December 3, 2019 15:47

update comment

cd17604

Co-Authored-By: Chris Kleinknecht <[email protected]>

don't need CURRENT_SPAN in the DefaultTracer

0afd484

Signed-off-by: Alex Boten <[email protected]>

Adding link to docs (open-telemetry#312)

b9ffc61

Fixes open-telemetry#245

Merge branch 'issue_166' of github.com:codeboten/opentelemetry-python…

2595251

… into issue_166

c24t approved these changes Dec 5, 2019

View reviewed changes

Oberon00 reviewed Dec 6, 2019

View reviewed changes

Oberon00 mentioned this pull request Dec 6, 2019

Consider using ABC #66

Closed

toumorokoshi approved these changes Dec 6, 2019

View reviewed changes

alrex added 2 commits December 6, 2019 11:31

Merge branch 'master' into issue_166

46ec642

updating tests

607ff4b

Signed-off-by: Alex Boten <[email protected]>

toumorokoshi added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 9, 2019

Oberon00 removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 9, 2019

lzchen reviewed Dec 9, 2019

View reviewed changes

a-feld removed their request for review December 10, 2019 00:54

Merge remote-tracking branch 'origin/master' into issue_166

fe0037a

Updated TracerSource to also use ABC, created the DefaultTracerSource no-op implementation. Signed-off-by: Alex Boten <[email protected]>

codeboten requested a review from a team January 9, 2020 18:52

c24t approved these changes Jan 9, 2020

View reviewed changes

forgot to make get_label_set abstract, removing implementations in ABC

bcaaf6b

Signed-off-by: Alex Boten <[email protected]>

c24t merged commit ada53ff into open-telemetry:master Jan 15, 2020

c24t mentioned this pull request Jan 22, 2020

Metrics export pipeline + metrics stdout exporter #341

Merged

GoPavel mentioned this pull request Jan 30, 2020

Missing ABC for Metric #390

Closed

This was referenced Feb 3, 2020

Make meter loader work with with meter interface #394

Closed

Make default meter a DefaultMeter lzchen/opentelemetry-python#7

Merged

codeboten deleted the issue_166 branch October 9, 2020 18:50

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

fix: pluginLoader for core modules (open-telemetry#312)

cc8a27d

closes open-telemetry#311 Signed-off-by: Olivier Albertini <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate no-op from interfaces #311

Separate no-op from interfaces #311

codeboten commented Nov 28, 2019 •

edited by c24t

Loading

codecov-io commented Nov 28, 2019 •

edited

Loading

codeboten commented Dec 3, 2019

c24t left a comment

c24t Dec 3, 2019

Oberon00 Dec 6, 2019

c24t Dec 3, 2019

codeboten Dec 3, 2019

Oberon00 Dec 6, 2019

Oberon00 Dec 6, 2019 •

edited

Loading

toumorokoshi Dec 6, 2019

carlosalberto Dec 6, 2019

c24t Dec 3, 2019

codeboten Dec 3, 2019

Oberon00 Dec 6, 2019

c24t left a comment

Oberon00 left a comment

toumorokoshi left a comment

toumorokoshi Dec 6, 2019

Oberon00 commented Dec 9, 2019

lzchen Dec 9, 2019

codeboten Jan 10, 2020

c24t left a comment

c24t Jan 9, 2020

codeboten Jan 10, 2020

c24t Jan 9, 2020

c24t Jan 9, 2020

	def _load_impl(
	api_type: Type[_T], factory: Optional[Callable[[Type[_T]], Optional[_T]]]
	) -> _T:
	"""Tries to load a configured implementation, if unsuccessful, returns a
	fast no-op implemenation that is always available.
	"""

	result = _try_load_configured_impl(api_type, factory)
	if result is None:
	return api_type()
	return result

	_TRACER = loader._load_impl(Tracer, _TRACER_FACTORY) # type: ignore
	_TRACER = loader._load_impl(DefaultTracer, _TRACER_FACTORY) # type: ignore

		@@ -175,12 +178,14 @@ def get_context(self) -> "SpanContext":
		# pylint: disable=no-self-use
		return INVALID_SPAN_CONTEXT

	"""The default TracerSource, used when no TracerSource implementation is available.
	"""The default TracerSource, used when no implementation is available.

Separate no-op from interfaces #311

Separate no-op from interfaces #311

Conversation

codeboten commented Nov 28, 2019 • edited by c24t Loading

codecov-io commented Nov 28, 2019 • edited Loading

Codecov Report

codeboten commented Dec 3, 2019

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

Oberon00 left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten commented Nov 28, 2019 •

edited by c24t

Loading

codecov-io commented Nov 28, 2019 •

edited

Loading

Oberon00 Dec 6, 2019 •

edited

Loading