-
Notifications
You must be signed in to change notification settings - Fork 657
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
Return proxy instruments from ProxyMeter #2169
Return proxy instruments from ProxyMeter #2169
Conversation
66b6a15
to
0d49d0f
Compare
0d49d0f
to
ff0c332
Compare
) | ||
return ProxyMeter(name, version=version, schema_url=schema_url) | ||
with self._lock: | ||
if self._real_meter_provider: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self._real_meter_provider: | |
if self._real_meter_provider is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be equivalent here right? I'm happy to change it if this is a general thing we enforce throughout our code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the time it is not a big deal, but it can be. It is part of PEP8 because of this reason:
Also, beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!
I don't think we have been consistent on enforcing this, though.
Should wait on #2182 to be merged before this. |
Should be good to go now 👍 |
Hm, looks like tests are still failing... |
@ocelotl failures are in contrib builds related to the release
I think #2199 should fix it? |
* Adds metrics API (#1887) * Adds metric prototype Fixes #1835 * Fix docs * Add API metrics doc * Add missing docs * Add files * Adding docs * Refactor to _initialize * Refactor initialize * Add more documentation * Add exporter test * Add process * Fix tests * Try to add aggregator_class argument Tests are failing here * Fix instrument parent classes * Test default aggregator * WIP * Add prototype test * Tests passing again * Use right counters * All tests passing * Rearrange instrument storage * Fix tests * Add HTTP server test * WIP * WIP * Add prototype * WIP * Fail the test * WIP * WIP * WIP * WIP * Add views * Discard instruments via views * Fix tests * WIP * WIP * Fix lint * WIP * Fix test * Fix lint * Fix method * Fix lint * Mypy workaround * Skip if 3.6 * Fix lint * Add reason * Fix 3.6 * Fix run * Fix lint * Remove SDK metrics * Remove SDK docs * Remove metrics * Remove assertnotraises mixin * Revert sdk docs conf * Remove SDK env var changes * Fix unit checking * Define positional-only arguments * Add Metrics plans * Add API tests * WIP * WIP test * WIP * WIP * WIP * Set provider test passing * Use a fixture * Add test for get_provider * Rename tests * WIP * WIP * WIP * WIP * Remove non specific requirement * Add meter requirements * Put all meter provider tests in one file * Add meter tests * Make attributes be passed as a dictionary * Make some interfaces private * Log an error instead * Remove ASCII flag * Add CHANGELOG entry * Add instrument tests * All tests passing * Add test * Add name tests * Add unit tests * Add description tests * Add counter tests * Add more tests * Add Histogram tests * Add observable gauge tests * Add updowncounter tests * Add observableupdowncounter tests * Fix lint * Fix docs * Fix lint * Ignore mypy * Remove useless pylint skip * Remove useless pylint skip * Remove useless pylint skip * Remove useless pylint skip * Remove useless pylint skip * Add locks to meter and meterprovider * Add lock to instruments * Fix fixmes * Fix lint * Add documentation placeholder * Remove blank line as requested. * Do not override Rlock * Remove unecessary super calls * Add missing super calls * Remove plan files * Add missing parameters * Rename observe to callback * Fix lint * Rename to secure_instrument_name * Remove locks * Fix lint * Remove args and kwargs * Remove implementation that gives meters access to meter provider * Allow creating async instruments with either a callback function or generator * add additional test with callback form of observable counter * add a test/example that reads measurements from proc stat * implement cpu time integration test with generator too Co-authored-by: Aaron Abbott <[email protected]> * Make measurement a concrete class (#2153) * Make Measurement a concrete class * comments * update changelog * Return proxy instruments from ProxyMeter (#2169) * Merge main 4 (#2236) * Add MeterProvider and Meter to the SDK Fixes #2200 * Add FIXMEs * Fix docstring * Add FIXME * Fix meter return * Log an error if a force flush fails * Add FIXME * Fix lint * Remove SDK API module * Unregister * Fix API names * Return _DefaultMeter * Remove properties * Pass MeterProvider as a parameter to __init__ * Add FIXMEs * Add FIXMEs * Fix lint * Add Aggregation to the metrics SDK Fixes #2229 * lint fix wip * Fix lint * Add proto to setup.cfg * Add timestamp for last value * Rename modules to be private * Fix paths * Set value in concrete classes init * Fix test * Fix lint * Remove temporalities * Use frozenset as key * Test instruments * Handle min, max and sum in explicit bucket histogram aggregator * Add test for negative values * Remove collect method from aggregations * Add make_point_and_reset * Remove add implementation * Remove _Synchronous * Update opentelemetry-sdk/src/opentelemetry/sdk/_metrics/aggregation.py Co-authored-by: Aaron Abbott <[email protected]> * Requested fixes * Remove NoneAggregation * Add changelog entry * Fix tests * Fix boundaries * More fixes * Update CHANGELOG.md Co-authored-by: Srikanth Chekuri <[email protected]> Co-authored-by: Aaron Abbott <[email protected]> Co-authored-by: Srikanth Chekuri <[email protected]>
Fixes #2144
This PR implements proxy instruments to be returned from the metrics API before an SDK is set up. This is slightly different than how this is implemented for tracing because of the complication of asynchronous instruments. In tracing, we resolve the proxied Tracer whenever spans are created by a user:
opentelemetry-python/opentelemetry-api/src/opentelemetry/trace/__init__.py
Lines 411 to 415 in 18b5cb0
This would work for synchronous instruments. However, the user never interacts with asynchronous instruments, so we need to keep them reachable by reference so when an SDK is swapped in, we can create SDK instances of each async instrument.