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

Add start_pipeline to MeterProvider in SDK, atexit moved to MeterProvider #791

Merged
merged 17 commits into from
Jun 9, 2020

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Jun 9, 2020

Addresses [#750]

Introduces a way to start the collection/export process via start_pipeline as part of the MeterProvider.
See below for usage:

metrics.set_meter_provider(MeterProvider())
meter = metrics.get_meter(__name__)
metrics.get_meter_provider().start_pipeline(meter, ConsoleMetricsExporter(), 5)

This makes it so that the user does not have to instantiate a PushController instance themselves (a concept that is quite confusing) and allows the user a mechanism to explicit say when they want to start collecting/exporting metrics. This makes it so that we do not have to worry about any metrics (or views in the future) defined during/after a collection has already started.

As well, the atexit logic initially part of PushController now belongs to the MeterProvider, which will call shutdown on all controllers and exporters upon application exit.

I'm using "pipeline" as the term based off the Go implementation but open to other suggestions.

@lzchen lzchen added sdk Affects the SDK package. metrics needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels Jun 9, 2020
@lzchen lzchen requested a review from a team as a code owner June 9, 2020 07:04
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.

LGTM 👍

opentelemetry-sdk/tests/metrics/test_metrics.py Outdated Show resolved Hide resolved
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.

Thanks! The API change looks good to me too: legible.

are there any other examples that should be modified as a result of this?

Also one comment on a loose atexit handler in the actual library code.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Adding a suggestion for default controllers

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

LGTM

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.

nitpick, thanks!

opentelemetry-sdk/tests/metrics/test_metrics.py Outdated Show resolved Hide resolved
@lzchen lzchen merged commit 672a89c into open-telemetry:master Jun 9, 2020
@lzchen lzchen deleted the controller branch June 9, 2020 23:33
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Jun 11, 2020
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Jun 11, 2020
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
Labels
metrics needs reviewers PRs with this label are ready for review and needs people to review to move forward. sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants