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

Resources sdk #464

Merged
merged 5 commits into from
Mar 9, 2020
Merged

Resources sdk #464

merged 5 commits into from
Mar 9, 2020

Conversation

mauriciovasquezbernal
Copy link
Member

Fixes #456.

This PR updates the resources class:

  • Move it to the SDK
  • Accept int, float, str, bool
  • Bind resource to MeterProvider and TracerProvider

It is still missing how exporters should export this value.

@mauriciovasquezbernal
Update resources to match spec
556b7b8 
- Move to SDK
- Accept int, float, str, bool
- Implement fast create for empty resource
@mauriciovasquezbernal
sdk: pass resource to spans
b6d6705
@mauriciovasquezbernal
sdk: add resource to meters
36a2fc0
@mauriciovasquezbernal mauriciovasquezbernal requested a review from a team March 5, 2020 01:25
toumorokoshi
toumorokoshi suggested changes Mar 5, 2020
View reviewed changes
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.

Question about the mocking creation of Resource strategy.

But my main blocking comment is around ensuring that Resources are indeed immutable, if they're used as default arguments.

opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py
return Resource(labels)

@staticmethod
def create_empty() -> "Resource":
return _EMPTY_RESOURCE
Copy link
Member

Choose a reason for hiding this comment

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

one thing about Resource is it's not exactly immutable right now, since the labels property returns the underlying container as an optimization.

Since we don't know how labels will be used anyway, should we just create a copy of the labels for now? That'll make sure the code above works.

Otherwise, all the default references to the empty Resource should be replaced with a constructor for a blank resource.

opentelemetry-sdk/tests/metrics/test_metrics.py Show resolved Hide resolved
opentelemetry-sdk/tests/trace/test_trace.py Outdated Show resolved Hide resolved
@mauriciovasquezbernal
update pr
48fddee 
- make Resource immutable
- use actual Resource object instead of mock
@mauriciovasquezbernal
Copy link
Member Author

@toumorokoshi I updated the Resource to be immutable. The labels passed are not copied, so a modification to the original label set could change the state of the Resource, do you think we should include a labels.copy() on the Resource constructor as well?

@codecov-io
Copy link

codecov-io commented Mar 5, 2020 •
edited
Loading

Codecov Report

Merging #464 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #464     +/-   ##
=========================================
+ Coverage   88.78%   88.98%   +0.2%     
=========================================
  Files          42       43      +1     
  Lines        2167     2216     +49     
  Branches      246      248      +2     
=========================================
+ Hits         1924     1972     +48     
- Misses        170      173      +3     
+ Partials       73       71      -2
Impacted Files Coverage Δ
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py 94.51% <100%> (+0.13%) ⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 92.51% <100%> (+0.45%) ⬆️
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py 92.85% <100%> (+22.02%) ⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py 68.14% <0%> (-0.05%) ⬇️
...opentelemetry/sdk/context/propagation/b3_format.py 87.27% <0%> (ø) ⬆️
...pentelemetry/context/propagation/httptextformat.py
.../context/propagation/tracecontexthttptextformat.py
.../opentelemetry/context/propagation/binaryformat.py
.../src/opentelemetry/context/propagation/__init__.py
...pi/src/opentelemetry/trace/propagation/__init__.py 84.61% <0%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 005575e...6b7c5af. Read the comment docs.

@mauriciovasquezbernal mauriciovasquezbernal added sdk Affects the SDK package. needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels Mar 9, 2020
@toumorokoshi
Copy link
Member

@toumorokoshi I updated the Resource to be immutable. The labels passed are not copied, so a modification to the original label set could change the state of the Resource, do you think we should include a labels.copy() on the Resource constructor as well?

possibly, it does leak state. Resources should be created relatively infrequently (once per app startup), so it's a good idea.

toumorokoshi
toumorokoshi approved these changes Mar 9, 2020
View reviewed changes
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.

LGTM, thanks!

@toumorokoshi toumorokoshi merged commit d7d9b15 into open-telemetry:master Mar 9, 2020
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.
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/resources-sdk branch April 14, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c24t c24t c24t approved these changes

@toumorokoshi toumorokoshi toumorokoshi approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Resources SDK
4 participants
@mauriciovasquezbernal @codecov-io @toumorokoshi @c24t