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.

Sign up for GitHub

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

Jump to bottom

Add a configuration manager #466

Merged
merged 28 commits into from
Mar 14, 2020
Merged

Add a configuration manager #466

merged 28 commits into from
Mar 14, 2020

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Mar 5, 2020 •
edited
Loading

Fixes #123

This basically removes loader.py (uses entry points instead) and all the calls for set_preferred_implementation* in order to cleanly separate user code from configuration. It introduces a configuration manager that can have configuration set by a configuration file (for the moment just a JSON file, probably a more suitable format would be ideal) or environment variables that override the former configuration method.

A couple test environments are still failing (mypy and tracecontext). Opening still to receive input.

@ocelotl ocelotl self-assigned this Mar 5, 2020
@ocelotl ocelotl force-pushed the issue_123 branch 18 times, most recently from 88f47e9 to fdcc761 Compare March 9, 2020 06:34
@ocelotl ocelotl changed the title Issue 123 Add a configuration manager Mar 9, 2020
@ocelotl ocelotl force-pushed the issue_123 branch 6 times, most recently from 34ec861 to 3354416 Compare March 10, 2020 00:33
@ocelotl ocelotl marked this pull request as ready for review March 10, 2020 00:44
@ocelotl ocelotl requested a review from a team March 10, 2020 00:44
@ocelotl ocelotl added api Affects the API package. enhancement New feature or request labels Mar 10, 2020
c24t
c24t approved these changes Mar 14, 2020
View reviewed changes
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 two questions:

I see examples that used to call set_preferred_implementation with an SDK object don't call set_provider now. Is this because you expect users to set the SDK objects in config instead? If so we probably need to include that in the docs.

Many tests set providers, but don't unset them. It looks like this isn't a problem in practice because tests that rely on a certain provider set it themselves, but we should avoid side effects like this.

opentelemetry-api/setup.py
@@ -12,14 +12,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import os
from os.path import dirname, join
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

opentelemetry-api/src/opentelemetry/configuration/__init__.py Outdated Show resolved Hide resolved
examples/metrics/collector.py
from opentelemetry.sdk.metrics.export.controller import PushController

# Meter is responsible for creating and recording metrics
metrics.set_preferred_meter_provider_implementation(lambda _: MeterProvider())
meter = metrics.get_meter(__name__)
Copy link
Member

Choose a reason for hiding this comment

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

This returns an api DefaultMeter now instead of a sdk Meter, shouldn't we have this line above?

metrics.set_meter_provider(sdk.metrics.MeterProvider())

Or is it intentional that the examples only use the API types? I think it'd be more helpful to show using the SDK since that's how most users will do it.

ext/opentelemetry-ext-otcollector/tests/test_otcollector_metrics_exporter.py
(
metrics._METER_PROVIDER,
metrics._METER_PROVIDER_FACTORY,
) = cls._meter_defaults
Copy link
Member

Choose a reason for hiding this comment

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

It seems like tests that set the meter/tracer provider on setup should still unset it on teardown. Why remove this instead of update it?

@c24t
Copy link
Member

c24t commented Mar 14, 2020

We've got two approvals and it looks like @ocelotl has addressed all the open comments, fixing a typo and merging.

@c24t c24t mentioned this pull request Mar 14, 2020
Open
@c24t c24t merged commit 68e909b into open-telemetry:master Mar 14, 2020
This was referenced Mar 14, 2020
Merged
Merged
c24t added a commit to c24t/opentelemetry-python that referenced this pull request Mar 14, 2020
@c24t
Fix versionchanged doc from open-telemetry#466
5d7f007
c24t added a commit that referenced this pull request Mar 14, 2020
@c24t @ocelotl @mauriciovasquezbernal
Config module doc fixes (#487)
71c0c83 
Add the new configuration module from #466 to the generated docs.

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>
c24t added a commit that referenced this pull request Mar 14, 2020
@c24t
Fix versionchanged doc from #466 (#489)
f52468b
mauriciovasquezbernal
mauriciovasquezbernal reviewed Mar 16, 2020
View reviewed changes
docs/examples/http/server.py
tracer = trace.get_tracer(__name__)

# SpanExporter receives the spans and send them to the target location.
span_processor = BatchExportSpanProcessor(exporter)
trace.tracer_provider().add_span_processor(span_processor)
trace.set_tracer_provider(TracerProvider())
Copy link
Member

Choose a reason for hiding this comment

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

This should be done before L42, otherwise the returned tracer will be de default one. I pushed a fix as a part of a747b61.

This was referenced Mar 16, 2020
Closed
Closed
toumorokoshi added a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 16, 2020
@toumorokoshi
Adding changelog entries for the 0.5b0 release
664b86d 
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
@toumorokoshi toumorokoshi mentioned this pull request Mar 16, 2020
Merged
c24t pushed a commit to c24t/opentelemetry-python that referenced this pull request Mar 16, 2020
@toumorokoshi @c24t
Adding changelog entries for the 0.5b0 release
8004571 
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
@c24t c24t mentioned this pull request Mar 18, 2020
Closed
@ocelotl ocelotl mentioned this pull request Apr 8, 2020
Closed
This was referenced Jun 2, 2020
Closed
Closed
@c24t c24t mentioned this pull request Jun 6, 2020
Merged
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@obecny @mayurkale22
chore: fixing coverage for karma using istanbul (open-telemetry#466)
ae904d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten left review comments

@mauriciovasquezbernal mauriciovasquezbernal mauriciovasquezbernal left review comments

@toumorokoshi toumorokoshi toumorokoshi approved these changes

@c24t c24t c24t approved these changes

Assignees

@ocelotl ocelotl

Labels
api Affects the API package. enhancement New feature or request sdk Affects the SDK package. usability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Global object for configuration
6 participants
@ocelotl @codecov-io @mauriciovasquezbernal @c24t @toumorokoshi @codeboten