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 MinMaxSumCount aggregator #422

Conversation

mauriciovasquezbernal
Copy link
Member

MinMaxSumCount is the default aggregator for Measure metrics: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#aggregations.

This PR implements this aggregator, updates some tests and reworks the example to use it.

The passed class is not the exact time defined in the API but a subclass.
Use issubclass instead of equal comparison.
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/implement-minmaxsumcount-aggregator branch from f6d7b35 to fa9b6dd Compare February 14, 2020 19:52
@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Attention: Patch coverage is 81.81818% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.20%. Comparing base (96943b2) to head (7b8ff91).

Files with missing lines Patch % Lines
.../src/opentelemetry/sdk/metrics/export/aggregate.py 79.31% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage   88.22%   88.20%   -0.02%     
==========================================
  Files          42       42              
  Lines        2047     2078      +31     
  Branches      233      237       +4     
==========================================
+ Hits         1806     1833      +27     
- Misses        170      172       +2     
- Partials       71       73       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/implement-minmaxsumcount-aggregator branch from fa9b6dd to d041411 Compare February 17, 2020 14:15
This aggregator is the default aggregator for measure metrics and
keeps the minimum, maximum, sum and count of those measures.
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/implement-minmaxsumcount-aggregator branch from d041411 to 7b8ff91 Compare February 17, 2020 16:23
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! Based on what I read from the spec, this looks accurate.

Random question: what's the thoughts around ConsoleMetricExporter taking in the minmaxsumcount value as it's own type? Is the expectation that the exporters will know how to handle custom data structures like this?

I imagine when this does get published to some downstream system, we would want those values encoded as separate metrics, with the category (min/max) included as either a tag or part of the metric name.

@mauriciovasquezbernal
Copy link
Member Author

I think we can expect that the logic to interpret the different data types is going to be on the exporters. For instance, the Prometheus exporter PR already has some logic to understand what is the type of metric to be exported, probably this logic will also have to be extended for supporting metrics with different aggregators.

@toumorokoshi
Copy link
Member

I think we can expect that the logic to interpret the different data types is going to be on the exporters. For instance, the Prometheus exporter PR already has some logic to understand what is the type of metric to be exported, probably this logic will also have to be extended for supporting metrics with different aggregators.

Yes, it's the aggregators I'm wondering about. It seems like there will be a need to create more aggregators based on various needs (e.g. histograms), and I think it's valuable to have an interface that enables exporters to understand what all the individual metrics to publish are, and what differentiates them so that information can be added appropriately as tags or as part of the metric.

# Update the metric instruments using the direct calling convention
requests_size.record(100, staging_label_set)
requests_counter.add(25, staging_label_set)
# Sleep for 5 seconds, exported value should be 25
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the comment to apply to all metrics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ops, I didn't have the time to update it before it was merged, will update in follow up PRs!

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!

@lzchen
Copy link
Contributor

lzchen commented Feb 18, 2020

Part of [#413]

@toumorokoshi
Copy link
Member

Thanks!

toumorokoshi pushed a commit that referenced this pull request Mar 1, 2020
…or (#431)

Implements #221.
Also fixes #394.

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 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 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-minmaxsumcount-aggregator branch April 14, 2020 21:50
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.

4 participants