Metrics API RFC 0003 cont'd #136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comments for Meter More comments Add more comments Fix typos
lzchen
requested review from
a-feld,
c24t,
carlosalberto,
Oberon00,
reyang and
toumorokoshi
as code owners
Merged
c24t
approved these changes
Sep 17, 2019
There was a problem hiding this comment.
This looks like a faithful implementation of the RFCs. I'll reserve judgement on "bidirectional" and comment on the spec or RFC instead.
For others, for reference, this is one of two metrics prototypes proving some in-flight RFCs. See the discussion at open-telemetry/opentelemetry-go#100 for more detail.
| bidirectional: bool = False, | ||
| label_keys: List[str] = None, | ||
| span_context: SpanContext = None, | ||
| ) -> Union["FloatCounter", "IntCounter"]: |
There was a problem hiding this comment.
It's a bit weird for the metric creation methods to return different types based on value_type instead of using a single generic type here, but neither solution is beautiful.
There was a problem hiding this comment.
You're right. It would actually be better to have one generic create method, but with the introduction of bidirectional, the metric types actually have slightly different behaviors.
toumorokoshi
approved these changes
Sep 17, 2019
| # TODO: RecordBatch | ||
|
|
||
|
|
||
| class ValueType(Enum): |
There was a problem hiding this comment.
do we need these? we could just use float and int, although this would require us to have other constants for datatypes that are not native to python.
There was a problem hiding this comment.
I'm not sure how to restrict the class parameter to only float and int, so an enum seemed like a good workaround.
| description: str, | ||
| unit: str, | ||
| value_type: "ValueType", | ||
| non_negative: bool = False, |
There was a problem hiding this comment.
for booleans, I like adding a more grammatically correct variable. e.g. "is_non_negative"
| INT = 1 | ||
|
|
||
|
|
||
| def create_counter( |
There was a problem hiding this comment.
Are these static now? I feel like it should be in some Meter object.
There was a problem hiding this comment.
Good question. The rationalization behind having the creates as a module level function is that we do not want to depend on the user having installed/initialized the default meter before using it.
There was a problem hiding this comment.
What about creating the global Meter lazily? That would still let users configure Meter at runtime as long as they do so before the first call. I think it's a worthwhile goal to have a working default Tracer and Meter out of the box, without any user-provided config.
There was a problem hiding this comment.
When you say lazily, does that mean instantiating a Meter when the API is imported? Similarly for tracer:
_TRACER = None # type: typing.Optional[Tracer]
_TRACER_FACTORY = None # type: typing.Optional[ImplementationFactory]
would be
_TRACER = Tracer() # type: typing.Optional[Tracer]
_TRACER_FACTORY = None # type: typing.Optional[ImplementationFactory]
Something like this?
Closed
hectorhdzg
added a commit
to hectorhdzg/opentelemetry-python
that referenced
this pull request
Oct 21, 2019
for start_time and end_time Make lint happy Addressing comments Addressing comments Allowing 0 as start and end time Fix lint issues Metrics API RFC 0003 cont'd (open-telemetry#136) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * address comments * fix comments Adding a working propagator, adding to integrations and example (open-telemetry#137) Adding a full, end-to-end example of propagation at work in the example application, including a test. Adding the use of propagators into the integrations. Metrics API RFC 0009 (open-telemetry#140) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint Console exporter (open-telemetry#156) Make use_span more flexible (closes open-telemetry#147). (open-telemetry#154) Co-Authored-By: Reiley Yang <[email protected]> Co-Authored-By: Chris Kleinknecht <[email protected]> WSGI fixes (open-telemetry#148) Fix http.url. Don't delay calling wrapped app. Skeleton for azure monitor exporters (open-telemetry#151) Add link to docs to README (open-telemetry#170) Move example app to the examples folder (open-telemetry#172) WSGI: Fix port 80 always appended in http.host (open-telemetry#173) Build and host docs via github action (open-telemetry#167) Add missing license boilerplate to a few files (open-telemetry#176) sdk/trace/exporters: add batch span processor exporter (open-telemetry#153) The exporters specification states that two built-in span processors should be implemented, the simple processor span and the batch processor span. This commit implements the latter, it is mainly based on the opentelemetry/java one. The algorithm implements the following logic: - a condition variable is used to notify the worker thread in case the queue is half full, so that exporting can start before the queue gets full and spans are dropped. - export is called each schedule_delay_millis if there is a least one new span to export. - when the processor is shutdown all remaining spans are exported. Implementing W3C TraceContext (fixes open-telemetry#116) (open-telemetry#180) * Implementing TraceContext (fixes open-telemetry#116) This introduces a w3c TraceContext propagator, primarily inspired by opencensus. fix time conversion bug (open-telemetry#182) Introduce Context.suppress_instrumentation (open-telemetry#181) Metrics Implementation (open-telemetry#160) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint * sdk * metrics * example * counter * Tests * Address comments * ADd tests * Fix typing and examples * black * fix lint * remove override * Fix tests * mypy * fix lint * fix type * fix typing * fix tests * isort * isort * isort * isort * noop * lint * lint * fix tuple typing * fix type * black * address comments * fix type * fix lint * remove imports * default tests * fix lint * usse sequence * remove ellipses * remove ellipses * black * Fix typo * fix example * fix type * fix type * address comments Implement Azure Monitor Exporter (open-telemetry#175) Span add override parameters for start_time and end_time (open-telemetry#179) CONTRIBUTING.md: Fix clone URL (open-telemetry#177) Add B3 exporter to alpha release table (open-telemetry#164) Update README for alpha release (open-telemetry#189) Update Contributing.md doc (open-telemetry#194) Add **simple** client/server examples (open-telemetry#191) Remove unused dev-requirements.txt (open-telemetry#200) The requirements are contained in tox.ini now. Fx bug in BoundedList for Python 3.4 and add tests (open-telemetry#199) * fix bug in BoundedList for python 3.4 and add tests collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future. Also, improve docstrings of BoundedList and BoundedDict classes Move util.time_ns to API. (open-telemetry#205) Add Jaeger exporter (open-telemetry#174) This adds a Jeager exporter for OpenTelemetry. This exporter is based on https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-jaeger. The exporter uses thrift and can be configured to send data to the agent and also to a remote collector. There is a long discussion going on about how to include generated files in the repo, so for now just put them here. Add code coverage Revert latest commit Fix some "errors" found by mypy. (open-telemetry#204) Fix some errors found by mypy (split from open-telemetry#201). Update README for new milestones (open-telemetry#218) Refactor current span handling for newly created spans. (open-telemetry#198) 1. Make Tracer.start_span() simply create and start the Span, without setting it as the current instance. 2. Add an extra Tracer.start_as_current_span() to create the Span and set it as the current instance automatically. Co-Authored-By: Chris Kleinknecht <[email protected]> Add set_status to Span (open-telemetry#213) Initial commit Initial version
srikanthccv
pushed a commit
to srikanthccv/opentelemetry-python
that referenced
this pull request
Nov 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes in this PR:
For 1, we do not want the creation of metrics to be dependent on whether or not the meter has been initialized yet (unlike in Java with global initialization).
For 2, as suggested by @toumorokoshi , it will be simpler to have less create methods for different metrics and pass in the data types as arguments. The reason I did not abstract it further down to metric TYPE (counter, gauge, measure) is due to the semantic and conceptually different behaviors of each, as well as the new addition of 4.
For 4, there was the desire to add optional parameters to the metric types for certain use cases. This is so we have an API that provides the functionality for all the uses cases but at the same time keeping the metric type definition simple. You can read more about it (here)[https://github.com/open-telemetry/oteps/blob/master/text/0003-measure-metric-type.md#cumulative-metric]
TODO:
TimeSeriestoHandle