Implement "named" meters + Remove "Batcher" from Meter constructor #431

lzchen · 2020-02-19T01:09:32Z

Implements [#221].
Also fixes [#394].

Follows this spec in regards to implementation, however uses MeterSource instead of MeterProvider to be consistent with TracerSource.

Also, stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of [#422] being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

toumorokoshi

A couple minor comments, but looks good overall!

One larger design question around APIs:

Binding How named meters to exporter

With named tracers, there's not much of an issue getting the traces out from them, as they are bound to a single exporter that is held by the TracerSource. In this implementation, there is no such binding, and there is also an additional layer binding the Meter to the Exporter with the PushController.

I think it's important to reduce friction in onboarding metrics from these systems. Is there a strategy there?

toumorokoshi · 2020-02-19T04:44:40Z

examples/metrics/record.py

 # Meter is responsible for creating and recording metrics
-metrics.set_preferred_meter_implementation(lambda _: Meter())
-meter = metrics.meter()
+meter = metrics.meter_source().get_meter(__name__)


In the same vein as #430, can we add metrics.get_meter()? Would save a lot of boilerplate and an abstraction many app developers will not have to deal with.

meter_source().get_meter() takes in an optional Batcher type in the constructor. How do I do the typing for this in the API without needing a dependency on the SDK where the Batcher class exists?

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

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

opentelemetry-sdk/src/opentelemetry/sdk/util.py

opentelemetry-api/tests/test_implementation.py

toumorokoshi · 2020-02-19T17:34:55Z

I think one other comment: Let's call "MeterSource" MeterProvider, as called out in the spec. I actually think this is something we need to rename on the tracer side too.

…try-python into named_meter

lzchen · 2020-02-19T22:39:25Z

@toumorokoshi
Can you explain more about why this is important? Why must there be a direct binding between the meters and the exporters that export the metrics?

codecov-io · 2020-02-19T23:43:17Z

Codecov Report

Merging #431 into master will increase coverage by 0.3%.
The diff coverage is 96.55%.

@@           Coverage Diff            @@
##           master    #431     +/-   ##
========================================
+ Coverage    88.5%   88.8%   +0.3%     
========================================
  Files          42      42             
  Lines        2105    2109      +4     
  Branches      238     238             
========================================
+ Hits         1863    1873     +10     
+ Misses        169     166      -3     
+ Partials       73      70      -3

Impacted Files	Coverage Δ
...elemetry-api/src/opentelemetry/metrics/__init__.py	`84.81% <0%> (ø)`	⬆️
.../src/opentelemetry/sdk/metrics/export/aggregate.py	`100% <100%> (+11.53%)`	⬆️
...opentelemetry/sdk/context/propagation/b3_format.py	`87.27% <0%> (ø)`	⬆️

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 f854923...3735d88. Read the comment docs.

…try-python into named_meter

lzchen · 2020-02-25T20:57:58Z

We are running into problems where the api layer takes dependencies on SDK classes (such as the Batcher in the constructor of Meter). With the discussions on open-telemetry/opentelemetry-specification#463 as well as the API/SDK separation issues with metrics, I am taking Batcher out of the parameters for the constructor of the Meter.

opentelemetry-sdk/src/opentelemetry/sdk/util/instrumentation.py

opentelemetry-sdk/tests/metrics/test_implementation.py

codeboten

Thanks for taking this on! Just a couple of nits and one change, but otherwise this is looking good. Only change is in the readme

ext/opentelemetry-ext-flask/tests/test_flask_integration.py

opentelemetry-api/src/opentelemetry/context/threadlocal_context.py

README.md

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

codeboten · 2020-02-27T17:00:42Z

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

+                exc_info=True,
+            )
+            _METER_SOURCE = DefaultMeterProvider()
+        del _METER_SOURCE_FACTORY


+1 on setting it to None, stays consistent with the _TRACER_PROVIDER_FACTORY

codeboten

LGTM

toumorokoshi

I think with a couple votes to do so, it's important to not "del" the METER_PROVIDER global.

Other than that, looks good!

toumorokoshi · 2020-02-28T20:46:37Z

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

-            _METER_SOURCE = DefaultMeterProvider()
-        del _METER_SOURCE_FACTORY
+            _METER_PROVIDER = DefaultMeterProvider()
+        del _METER_PROVIDER_FACTORY


can we set this to none?

toumorokoshi

Great, thanks!

Rename TracerSource to TracerProvider (open-telemetry#441) Following discussion in open-telemetry#434, align the name with the specification. Co-authored-by: Chris Kleinknecht <libc@google.com> Fix new ext READMEs (open-telemetry#444) Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with: HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/ Adding attach/detach methods as per spec (open-telemetry#429) This change updates the Context API with the following: - removes the remove_value method - removes the set_current method - adds attach and detach methods Fixes open-telemetry#420 Co-authored-by: Chris Kleinknecht <libc@google.com> Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439) OT Collector trace exporter (open-telemetry#405) Based on the OpenCensus agent exporter. Fixes open-telemetry#343 Co-authored-by: Chris Kleinknecht <libc@google.com> API: Renaming TraceOptions to TraceFlags (open-telemetry#450) Renaming TraceOptions to TraceFlags, which is the term used to describe the flags associated with the trace in the OpenTelemetry specification. Closes open-telemetry#434 api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431) Implements open-telemetry#221. Also fixes open-telemetry#394. Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them. Lastly, moves InstrumentationInfo from trace.py in the sdk to utils. Prepare to host on readthedocs.org (open-telemetry#452) sdk: Implement observer instrument (open-telemetry#425) Observer instruments are used to capture a current set of values at a point in time [1]. This commit extends the Meter interface to allow to register an observer instrument by pasing a callback that will be executed at collection time. The logic inside collection is updated to consider these instruments and a new ObserverAggregator is implemented. [1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments sdk: fix ConsoleSpanExporter (open-telemetry#455) 19d573a ("Add io and formatter options to console exporter (open-telemetry#412)") changed the way spans are printed by using write() instead of print(). In Python 3.x sys.stdout is line-buffered, so the spans were not being printed to the console at the right timing. This commit fixes that by adding an explicit flush() call at the end of the export function , it also changes the default formatter to include a line break. To be precise, only one of the changes was needed to solve the problem, but as a matter of completness both are included, i.e, to handle the case where the formatter chosen by the user doesn't append a line break. jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459) Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already. Users following the usage docs will currently run into this error: `AttributeError: 'Tracer' object has no attribute 'add_span_processor'` api: Implementing Propagators API to use Context (open-telemetry#446) Implementing Propagators API to use Context. Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation. Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces. Adding a composite propagator. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Total Changelog: Documentations has been significantly overhauled, including: * a getting started guide * examples has been consolidated to an docs/examples folder * several minor improvements to the examples in each extension's readme. - Adding Correlation Context API and propagator ([open-telemetry#471](open-telemetry#471)) - Adding a global configuration module to simplify setting and getting globals ([open-telemetry#466](open-telemetry#466)) - Rename metric handle to bound metric ([open-telemetry#470](open-telemetry#470)) - Moving resources to sdk ([open-telemetry#464](open-telemetry#464)) - Implementing propagators to API to use context ([open-telemetry#446](open-telemetry#446)) - Implement observer instrument for metrics ([open-telemetry#425](open-telemetry#425)) - Adding named meters, removing batchers ([open-telemetry#431](open-telemetry#431)) - Renaming TraceOptions to TraceFlags ([open-telemetry#450](open-telemetry#450)) - Renaming TracerSource to TraceProvider ([open-telemetry#441](open-telemetry#441)) - Adding Correlation Context SDK and propagator ([open-telemetry#471](open-telemetry#471)) - Adding OT Collector metrics exporter ([open-telemetry#454](open-telemetry#454)) - Improve validation of attributes ([open-telemetry#460](open-telemetry#460)) - Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span() (open-telemetry#469) ([open-telemetry#469](open-telemetry#469)) - Adding named meters, removing batchers ([open-telemetry#431](open-telemetry#431)) - Make counter and MinMaxSumCount aggregators thread safe ([open-telemetry#439](open-telemetry#439)) - Initial release. Support is included for both trace and metrics.

Rename TracerSource to TracerProvider (open-telemetry#441) Following discussion in open-telemetry#434, align the name with the specification. Co-authored-by: Chris Kleinknecht <libc@google.com> Fix new ext READMEs (open-telemetry#444) Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with: HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/ Adding attach/detach methods as per spec (open-telemetry#429) This change updates the Context API with the following: - removes the remove_value method - removes the set_current method - adds attach and detach methods Fixes open-telemetry#420 Co-authored-by: Chris Kleinknecht <libc@google.com> Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439) OT Collector trace exporter (open-telemetry#405) Based on the OpenCensus agent exporter. Fixes open-telemetry#343 Co-authored-by: Chris Kleinknecht <libc@google.com> API: Renaming TraceOptions to TraceFlags (open-telemetry#450) Renaming TraceOptions to TraceFlags, which is the term used to describe the flags associated with the trace in the OpenTelemetry specification. Closes open-telemetry#434 api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431) Implements open-telemetry#221. Also fixes open-telemetry#394. Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them. Lastly, moves InstrumentationInfo from trace.py in the sdk to utils. Prepare to host on readthedocs.org (open-telemetry#452) sdk: Implement observer instrument (open-telemetry#425) Observer instruments are used to capture a current set of values at a point in time [1]. This commit extends the Meter interface to allow to register an observer instrument by pasing a callback that will be executed at collection time. The logic inside collection is updated to consider these instruments and a new ObserverAggregator is implemented. [1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments sdk: fix ConsoleSpanExporter (open-telemetry#455) 19d573a ("Add io and formatter options to console exporter (open-telemetry#412)") changed the way spans are printed by using write() instead of print(). In Python 3.x sys.stdout is line-buffered, so the spans were not being printed to the console at the right timing. This commit fixes that by adding an explicit flush() call at the end of the export function , it also changes the default formatter to include a line break. To be precise, only one of the changes was needed to solve the problem, but as a matter of completness both are included, i.e, to handle the case where the formatter chosen by the user doesn't append a line break. jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459) Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already. Users following the usage docs will currently run into this error: `AttributeError: 'Tracer' object has no attribute 'add_span_processor'` api: Implementing Propagators API to use Context (open-telemetry#446) Implementing Propagators API to use Context. Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation. Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces. Adding a composite propagator. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

lzchen added 2 commits February 18, 2020 16:56

Named meters

bba1fea

Remove test

712ccd3

lzchen requested a review from a team February 19, 2020 01:09

fix comment

b50c269

lzchen mentioned this pull request Feb 19, 2020

Metric exporters to prepend name of meter to metric instruments #432

Closed

lzchen added the metrics label Feb 19, 2020

toumorokoshi suggested changes Feb 19, 2020

View reviewed changes

mauriciovasquezbernal reviewed Feb 19, 2020

View reviewed changes

opentelemetry-api/tests/test_implementation.py Outdated Show resolved Hide resolved

opentelemetry-api/tests/test_implementation.py Show resolved Hide resolved

lzchen added 3 commits February 19, 2020 10:30

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

fd3d175

…try-python into named_meter

Fix tests

08b1117

address comments + fix lint

c92f837

lzchen added 3 commits February 19, 2020 14:45

Rename MeterSource to MeterProvider

7b8b67d

black

63824f3

black

16e728b

remove black

7733a87

codeboten mentioned this pull request Feb 20, 2020

Beta Readiness Tracker #418

Closed

34 tasks

lzchen added 6 commits February 20, 2020 10:21

fix lint

b436c62

fix lint

d6c97f0

fix lint

a2f2e0f

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

32cf2c1

…try-python into named_meter

Merge

ec9c673

Rename to MeterProvider, remove Batcher from constructor

7ebd438

lzchen changed the title ~~Implement "named" meters~~ Implement "named" meters + Remove "Batcher" from Meter constructor Feb 25, 2020

lzchen added 3 commits February 25, 2020 13:23

fix mypy

27d75ba

black

08095b6

Black

6a743c7

codeboten reviewed Feb 27, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/util/instrumentation.py Outdated Show resolved Hide resolved

codeboten reviewed Feb 27, 2020

View reviewed changes

opentelemetry-sdk/tests/metrics/test_implementation.py Outdated Show resolved Hide resolved

black

4770ec4

codeboten suggested changes Feb 27, 2020

View reviewed changes

lzchen added 2 commits February 27, 2020 09:19

remove black

0f36b31

ADDRESS COMMENTS

4f4632b

codeboten approved these changes Feb 27, 2020

View reviewed changes

lzchen added 4 commits February 27, 2020 11:19

black

e591a39

fix lint

0878f5d

fix example

584d996

Fix lint

f854923

toumorokoshi added this to the Beta milestone Feb 27, 2020

lzchen requested a review from toumorokoshi February 27, 2020 23:14

lzchen added 3 commits February 27, 2020 18:30

merge

4df2553

fix labelset

40ee67b

fix lint

0b93285

toumorokoshi suggested changes Feb 28, 2020

View reviewed changes

set to none

3735d88

toumorokoshi approved these changes Mar 1, 2020

View reviewed changes

toumorokoshi merged commit 11467c4 into open-telemetry:master Mar 1, 2020

lzchen deleted the named_meter branch March 2, 2020 00:14

This was referenced Mar 2, 2020

Adding OT Collector metrics exporter #454

Merged

Implement "named" Meters in metrics #221

Closed

toumorokoshi mentioned this pull request Mar 16, 2020

Changelog and version updates for 0.5b0 release #498

Merged

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

chore: add prepare script and bump the version (open-telemetry#431)

4c0626c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement "named" meters + Remove "Batcher" from Meter constructor #431

Implement "named" meters + Remove "Batcher" from Meter constructor #431

lzchen commented Feb 19, 2020

toumorokoshi left a comment

toumorokoshi Feb 19, 2020

lzchen Feb 19, 2020

toumorokoshi commented Feb 19, 2020

lzchen commented Feb 19, 2020

codecov-io commented Feb 19, 2020 •

edited

Loading

lzchen commented Feb 25, 2020

codeboten left a comment

codeboten Feb 27, 2020

codeboten left a comment

toumorokoshi left a comment

toumorokoshi Feb 28, 2020

toumorokoshi left a comment

Implement "named" meters + Remove "Batcher" from Meter constructor #431

Implement "named" meters + Remove "Batcher" from Meter constructor #431

Conversation

lzchen commented Feb 19, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

Binding How named meters to exporter

toumorokoshi Feb 19, 2020

Choose a reason for hiding this comment

lzchen Feb 19, 2020

Choose a reason for hiding this comment

toumorokoshi commented Feb 19, 2020

lzchen commented Feb 19, 2020

codecov-io commented Feb 19, 2020 • edited Loading

Codecov Report

lzchen commented Feb 25, 2020

codeboten left a comment

Choose a reason for hiding this comment

codeboten Feb 27, 2020

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi Feb 28, 2020

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 19, 2020 •

edited

Loading