-
Notifications
You must be signed in to change notification settings - Fork 649
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
Adding the Resource API #61
Changes from 3 commits
0554ec3
e16e854
b95da6a
8beab78
389dde8
3c363c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,3 +11,45 @@ | |||||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
# See the License for the specific language governing permissions and | ||||||
# limitations under the License. | ||||||
from abc import ABC, abstractmethod | ||||||
from typing import Dict, Union | ||||||
|
||||||
|
||||||
class Resource(ABC): | ||||||
"""This the interface that resources must implement""" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "This the" -> "This is the"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. sorry I'll scan for grammatical errors myself as well. |
||||||
def __init__(self, labels: Dict[str, str]): | ||||||
toumorokoshi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
"""Construct a resource. | ||||||
|
||||||
Direct calling of the constructor is discouraged, as it cannot | ||||||
take advantage of caching and restricts to the type of object | ||||||
that can be returned. | ||||||
""" | ||||||
self._labels = labels | ||||||
|
||||||
@staticmethod | ||||||
def create(labels: Dict[str, str]) -> "Resource": | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know that we want to force implementers to have a static constructor; the arguments for java might not apply here. It would be great to have a static empty resource on this class though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a discussion in opentelemetry-specifications about this, and right now I'm a little convinced toward using a factory. There's a lot of functionality we can augment without adding significant code complexity (e.g. caching and a more flexible type signature. I think it is theoretically possible to accomplish something similar with constructors, but that requires some fairly complex concepts like metaclasses to pull off. DefaultResource doesn't take strong advantage of this, but I wonder if other resource implementations may. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds fine to me, and maybe we should have factories for more of these classes. Any reason to keep the constructor impl here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, sorry, that was a mistake. as was keeping some methods non-abstract. I'll address that. |
||||||
"""create a new resource. | ||||||
|
||||||
Args: | ||||||
labels: the labels that define the resource | ||||||
|
||||||
Returns: | ||||||
The resource with the labels in question | ||||||
""" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No blank lines here? I'm surprised lint doesn't complain about this one either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will add blank lines |
||||||
@property | ||||||
@abstractmethod | ||||||
def labels(self) -> Dict[str, str]: | ||||||
"""Return the label dictionary associated with this resource. | ||||||
|
||||||
Returns: | ||||||
A dictionary with the labels of the resource | ||||||
""" | ||||||
def merge(self, other: Union["Resource", None]) -> "Resource": | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will fix. |
||||||
"""Return a resource with the union of labels for both resources. | ||||||
|
||||||
Labels that exist in the main Resource take | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it doesn't make sense to say to follow PEP8 everywhere except for the bit about wrapping docstring text at 72 chars, but having two different text widths in the same file seems like an unnecessary hassle for no obvious benefit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, it wasn't an intentional choice. Generally I use black which makes that consistent. I tried yapf which does not handle this reformatting. I'll do some work to make sure I'm more consistent in the future. |
||||||
precedence unless the label value is the empty string. | ||||||
|
||||||
Args: | ||||||
other: the resource to merge in | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Or lowercase and lose the period in other docstrings. Same thing for the other args/returns in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry I'll try to be consistent in the future. Looking at Sphinx, it lists an example of Google style: http://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#google-vs-numpy Which capitalizes without a period for arguments. |
||||||
""" |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,44 @@ | ||||
# Copyright 2019, OpenTelemetry Authors | ||||
# | ||||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||||
# you may not use this file except in compliance with the License. | ||||
# You may obtain a copy of the License at | ||||
# | ||||
# http://www.apache.org/licenses/LICENSE-2.0 | ||||
# | ||||
# Unless required by applicable law or agreed to in writing, software | ||||
# distributed under the License is distributed on an "AS IS" BASIS, | ||||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||
# See the License for the specific language governing permissions and | ||||
# limitations under the License. | ||||
from typing import Any, Dict, Union | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: other files do keep a blank line after the copyright comment, it'll be great to keep consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for pointing that out! will do. A cursory glance doesn't show this capability, but I wonder if we can lint for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This line should fail lint since these are unused, but we weren't linting the SDK by default. I add the check in #65, which means this PR will fail the check once that PR is merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and if we start writing out own pylint checkers maybe that's a signal that we've got too far and should just start using a formatter. |
||||
from opentelemetry.resources import Resource | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be missing something here but is this import line correct? I'm getting an import error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in order for this to work, you must install the opentelemetry api package. For a reason which is not clear to me, API packages go in opentelemetry rather than opentelemetry.api. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's some discussion about We should probably add a contributing/developing doc to list gotchas like that you have to install the packages to get this to work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that would be great! the gotchas are fresh in my mind as well, I make try a PR |
||||
|
||||
|
||||
class DefaultResource(Resource): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, I can perform that pattern. Maybe a separate issue/pr since there's already a precedent, but I think it would still be beneficial to differentiate between the API and the Default. Maybe ResourceABC and Resource? or ResourceInterface? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a valid point, and it could be helpful if some error message only has class name but not the full module path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we go really old school we can follow the zope interface convention (and add the I): https://zopeinterface.readthedocs.io/en/latest/README.html#defining-interfaces I'll fill a separate issue for that, so we can carry that discussion separately. |
||||
def __init__(self, labels): | ||||
self._labels = labels | ||||
|
||||
@property | ||||
def labels(self): | ||||
return self._labels | ||||
|
||||
@staticmethod | ||||
def create(labels): | ||||
return DefaultResource(labels) | ||||
|
||||
def merge(self, other): | ||||
if other is None: | ||||
return self | ||||
if not self._labels: | ||||
return other | ||||
merged_labels = self.labels.copy() | ||||
for key, value in other.labels.items(): | ||||
if key not in merged_labels or merged_labels[key] == "": | ||||
merged_labels[key] = value | ||||
return DefaultResource(merged_labels) | ||||
|
||||
def __eq__(self, other: object) -> bool: | ||||
if not isinstance(other, DefaultResource): | ||||
return False | ||||
return self.labels == other.labels |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import unittest | ||
from opentelemetry.sdk.resources import DefaultResource | ||
|
||
|
||
class TestResources(unittest.TestCase): | ||
def test_resource_merge(self): | ||
left = DefaultResource({"service": "ui"}) | ||
right = DefaultResource({"host": "service-host"}) | ||
self.assertEqual( | ||
left.merge(right), | ||
DefaultResource({ | ||
"service": "ui", | ||
"host": "service-host" | ||
})) | ||
|
||
def test_resource_merge_empty_string(self): | ||
""" | ||
labels from the source Resource take | ||
precedence, with the exception of the empty string. | ||
""" | ||
left = DefaultResource({"service": "ui", "host": ""}) | ||
right = DefaultResource({"host": "service-host", "service": "not-ui"}) | ||
self.assertEqual( | ||
left.merge(right), | ||
DefaultResource({ | ||
"service": "ui", | ||
"host": "service-host" | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't been using
ABC
/abstractmethod
so far, but this if there's anywhere that it's a good idea, it's this package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not the biggest fan of ABC's, but they do provide a great signal as to whether someone has implemented the required interface or not.
I tend to use it only when I expect non-core maintainers to implement the interface.
one caveat I see is that, as we add new APIs, implementations will need to be fixed to support the new method, even if it isn't used. But hopefully the spec itself is fairly concrete, or we simply don't annotate those methods with abstractmethod.