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

Move "config" out from Meter into MeterProvider #751

Merged
merged 12 commits into from
Jun 4, 2020

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented May 29, 2020

Part of [#750]
Similar to how Tracer has a reference to TracerProvider and configuration are done through the TracerProvider and passed along to each Tracer created for it. Apply similar logic for stateful for MeterProvider and Meter.

The purpose of this in terms of SDK design is to move a lot of the configuration to one central place (MeterProvider), instead of giving responsibility to other components. The next logical step would be to move PushController as part of the MeterProvider and have MeterProvider control the config Dot Net is doing something like this). Another option is introduce a new "Pipeline" concept, in which we register each component to a Pipeline and that will be the path of logic that metrics will take. Go is doing something like this. Any suggestions not related to this PR please discuss in [#750].

@lzchen lzchen requested a review from a team May 29, 2020 03:08
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Had a quick question on stateful. This PR doesn't change this because stateful was passed into the Meter before, but why isn't the stateful bool scoped to the individual metrics? I could see a case where you might want to create stateful and stateless metrics.

# Stateful determines whether how metrics are collected: if true, metrics
# accumulate over the process lifetime. If false, metrics are reset at the
# beginning of each collection interval.
metrics.set_meter_provider(MeterProvider(batcher_mode == "stateful"))
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this check happens on line 46. Right now this script crashes without any arguments

Suggested change
metrics.set_meter_provider(MeterProvider(batcher_mode == "stateful"))
metrics.set_meter_provider(MeterProvider(stateful))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. The current design is such that the responsibility for "statefulness" belongs to the batcher, and in turn, the meter. This means that all metrics that created and recorded with this meter will have the same statefulness. This "batcher" concept was created previously when the metrics api was not as well defined (and we still don't really have a well defined SDK spec), in which we needed a way to determine whether we are calculating actual values or deltas. I think some of the optional refinements for metric instruments is trying to address these cases, in which if they become actual api elements, we will have to change our design to decouple statefulness from the batcher and make them a metric level control.

docs/examples/basic_meter/basic_metrics.py Show resolved Hide resolved
@lzchen lzchen added api Affects the API package. metrics labels Jun 1, 2020
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

The spirit of the change LGTM, but when can we remove stateful altogether?

@lzchen
Copy link
Contributor Author

lzchen commented Jun 2, 2020

@c24t
Good question. If the metric instruments in the otep become agreed upon in the specs, the statefulness will longer needed to be a batcher/meter concept, each metric instrument will simply have it's own default aggregation behaviour, eliminating the need for stateful for certain cases(e.g. cumulative for stateful, non-cumulative for non-stateful). There is still the case of stateful for measure instruments however. Could be a discussion for a different PR.

EDIT: Can we get a stamp on this?

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM, but ideally we get all these args out of MeterProvider and into shared config.

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.

great, thanks!

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Change looks good, just a minor nit on the changelog entry to call out resource as well.

opentelemetry-sdk/CHANGELOG.md Outdated Show resolved Hide resolved
@codeboten codeboten added this to the Beta v0.9 milestone Jun 4, 2020
@codeboten codeboten merged commit 75a1311 into open-telemetry:master Jun 4, 2020
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Jun 5, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* chore: 0.4.0 release proposal

* use api instead of types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants