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

Implement observer instrument #425

Merged

Conversation

mauriciovasquezbernal
Copy link
Member

This PR implements the Observer instrument: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer.

An Observer is created by calling the new register_observer method on Meter. This method receives a callback that is invoked on collection time with the Observer instrument as parameter, this provides the observe function for capturing data. It is not possible to unregister an Observer yet.

Observers are aggregated using a MinMaxSumCountLast aggregator based on #422.

Fixes #403
Fixes #178

The passed class is not the exact time defined in the API but a subclass.
Use issubclass instead of equal comparison.
This aggregator is the default aggregator for measure metrics and
keeps the minimum, maximum, sum and count of those measures.
This aggregator is the default aggregator for measure metrics and
keeps the minimum, maximum, sum and count of those measures.
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
@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@df01d09). Click here to learn what that means.
The diff coverage is 87.95%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #425   +/-   ##
=========================================
  Coverage          ?   88.63%           
=========================================
  Files             ?       42           
  Lines             ?     2165           
  Branches          ?      245           
=========================================
  Hits              ?     1919           
  Misses            ?      171           
  Partials          ?       75
Impacted Files Coverage Δ
opentelemetry-api/src/opentelemetry/util/loader.py 81.25% <ø> (ø)
...lemetry-sdk/src/opentelemetry/sdk/util/__init__.py 85.88% <ø> (ø)
...entelemetry-api/src/opentelemetry/util/__init__.py 83.33% <ø> (ø)
...try-ext-wsgi/src/opentelemetry/ext/wsgi/version.py 100% <100%> (ø)
...xt-zipkin/src/opentelemetry/ext/zipkin/__init__.py 84.41% <100%> (ø)
.../context/propagation/tracecontexthttptextformat.py 84.72% <100%> (ø)
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py 86.54% <100%> (ø)
...sdk/src/opentelemetry/sdk/trace/export/__init__.py 89.2% <100%> (ø)
...-ext-flask/src/opentelemetry/ext/flask/__init__.py 88.88% <100%> (ø)
...telemetry-api/src/opentelemetry/context/context.py 100% <100%> (ø)
... and 19 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 df01d09...f56e583. Read the comment docs.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the code changes look good. I'll wait for the conflict resolution before approving just to make sure I understand what has been added regarding testing the observe.

Aside from that, the API seems like it would make some scenarios very difficult for me to implement. I wonder if there's been discussion on this?

Batcher being attached to Meter.

It seems the batcher is implicitly responsible for choosing the aggregation, which also means that the batcher is responsible for choosing what sort of metrics to keep around the observations (min, max, count, etc).

I think it's a common scenario to want to have different types of metrics emitted for different types of observation. For example, if my observation is a single metric, there's no need for me to have a a min/max/count version of the metric. If I want to observe that single metric and, for example, CPU, where I want such groupings, I have a fairly complex path to actually configure that:

  1. create Exporter
  2. create BatcherA (single Aggregator), MeterA for my single metric
  3. create PushControllerA with MeterA and Exporter
  4. create BatcherB (sum/min/max/count Aggregator), MeterB for my second metric
  5. create PushControllerB with MeterB and Exporter

So, effectively I need 2-3 additional data structures (batcher, pushcontroller, meter) for every variant of Aggregator I want to support. It's a lot of overhead.

I'd argue (and I'm guessing this is a spec level conversation) that it should be possible to tie aggregators directly to the observer, so I can choose what statistics I want to track on my observer specifically for my observer.

Observers of different frequencies

It looks like the Observer run rate is dependent on how often the exporter is responsible for collecting data, but it could be possible that I want to observe some metrics at a different rate than others. For example, maybe I want to capture CPU/Memory on a per minute basis, but I want # of coroutines in an event loop faster, maybe every 30 seconds.

In an exporter like prometheus this is irrelevant as the rate is determined by the poller. However, there are plenty of exporters that have a push model, and many metric storage systems support different granularity of time series. Again, it's not great to require a separate meter, exporter, and pushcontroller to support different poll frequencies.

examples/metrics/observer_example.py Outdated Show resolved Hide resolved
- use explicit parameter names to make it more clear
- fix comments (actually remove them) in simple_example
@mauriciovasquezbernal
Copy link
Member Author

I'd argue (and I'm guessing this is a spec level conversation) that it should be possible to tie aggregators directly to the observer, so I can choose what statistics I want to track on my observer specifically for my observer.

It's a more general discussion that applies to other instruments as well, specially Measure that could support multiple types of aggregations.

Currently, the aggregator is chosen based on the instrument type:

def aggregator_for(self, metric_type: Type[MetricT]) -> Aggregator:
"""Returns an aggregator based on metric type.
Aggregators keep track of and updates values when metrics get updated.
"""
# pylint:disable=R0201
if issubclass(metric_type, Counter):
return CounterAggregator()
if issubclass(metric_type, Measure):
return MinMaxSumCountAggregator()
# TODO: Add other aggregators
return CounterAggregator()

As far as I know this is an ongoing discussion on the spec about how to choose the aggregator for a given instrument.

Again, it's not great to require a separate meter, exporter, and pushcontroller to support different poll frequencies.

I agree, the current architecture makes it very hard to support observers with different frequencies. I think this limitation comes from the fact that observers are run on collection time, that is determined by the controller. I'm not aware of any discussion about it.

I think these are discussions for the specification.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@lzchen
Copy link
Contributor

lzchen commented Feb 26, 2020

So, effectively I need 2-3 additional data structures (batcher, pushcontroller, meter) for every variant of Aggregator I want to support. It's a lot of overhead.

@toumorokoshi I am a bit confused as to why you need additional data structures for variants of Aggregators. Aggregators are generated from the metrics that require them, not from the type of Batcher.

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@toumorokoshi toumorokoshi added this to the Beta milestone Feb 27, 2020
@toumorokoshi toumorokoshi merged commit 888bed9 into open-telemetry:master Mar 2, 2020
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 6, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>
toumorokoshi added a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 16, 2020
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.
c24t pushed a commit to c24t/opentelemetry-python that referenced this pull request Mar 16, 2020
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.
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 17, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 26, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/implement-observer branch April 14, 2020 21:50
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* chore: update Release Schedule

* fix: minor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add observer, remove gauge Implement Metric Observer + Callbacks
4 participants