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

Adding the Resource API #61

Merged
merged 6 commits into from
Jul 29, 2019

Conversation

toumorokoshi
Copy link
Member

Hi! This is a first try at adding the resource API: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-resources.md

This branch currently fails due to conflicting linting and mypy rules:

  • mypy dictates that the Any type is unneeded, and should be elided.
  • pylint dictates that all arguments in methods / functions need a type.

When merging resources, the non-empty string value takes precedence.
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.

Thanks for doing this! The content looks good, but should be moved out of the API package.

We're formatting docstrings as:

    """A short, single-line description of the function.

    A longer description that may include :class:`.SphinxStuff` and can wrap
    and wrap and wrap.

    Args:
        arg: Short description, no need to include the type.

    Returns:
        The thing we return, or "Yields:" for context managers.
    """

This produces nice docs via sphinx, which you should be able to generate with tox -e docs.

opentelemetry-api/src/opentelemetry/resources/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/resources/__init__.py Outdated Show resolved Hide resolved
and as such it is not recommended to copy the
result if it is desired to mutate the result.
"""
return self._labels
Copy link
Member

Choose a reason for hiding this comment

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

Why give _labels the underscore treatment if this method just returns it directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

The desire was to match the API specification, which calls for GetLabels.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-resources.md#getlabels

That said now that I understand that the idea is to provide capabilities rather than specific method names / etc, I can just remove this and make labels public.

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched this over to a property. I was hoping that I could assign values directly and have that satisfy and ABC's property interface, but unfortunately that didn't work as expected.

I've tried to reach a middle ground in the code, which still has a private _labels dict. But it does expose labels in a more Pythonic fashion, while ensuring that any class extending the ABC will still have a labels property.

opentelemetry-api/tests/resources/test_init.py Outdated Show resolved Hide resolved
self._labels = labels

@staticmethod
def create(labels: Dict[str, str]) -> "Resource":
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@staticmethod
def create(labels: Dict[str, str]) -> "Resource":
"""
create a new resource. This is the recommended
Copy link
Member

Choose a reason for hiding this comment

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

minor: capitalize "Create"?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes! will do.

The API package should be minimal, and example implementations should
live in SDK when possible.

Fixing docstrings to match sphinx-compatible formatting.

using unittest-standard asserts to adhere to conventions.

switcihng from Any to object works helps pass linting and mypy
compatibility.
# 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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from typing import Any, Dict, Union

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.

Copy link
Member

Choose a reason for hiding this comment

The 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


class DefaultResource(Resource):
Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we name this class as Resource similar like what @c24t did for Span.

This makes sure a much cleaner callstack (I would struggle with name DefaultResource and DefaultSpan #63).

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.
In Java and .NET world I've seen something like Resource and ResourceImpl (I'm trying to avoid IResource here since Python doesn't really have interface concept). We can probably steal the idea (or peek at what other Python libs are doing).

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM after the class rename and styling fix.



class Resource(ABC):
"""This the interface that resources must implement"""
Copy link
Member

Choose a reason for hiding this comment

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

"This the" -> "This is the"?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed. sorry I'll scan for grammatical errors myself as well.

# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Any, Dict, Union
from opentelemetry.resources import Resource
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

There's some discussion about opentelemetry.api.trace vs opentelemetry.trace in #15 (comment) and #55 (comment), the tl;dr is we generally expect people to import the API only.

We should probably add a contributing/developing doc to list gotchas like that you have to install the packages to get this to work.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

from typing import Dict, Union


class Resource(ABC):
Copy link
Member

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.

Copy link
Member Author

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.

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 with some minor comments, renaming DefaultResource, and passing the new lint checks from #65.


Returns:
The resource with the labels in question
"""
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

will add blank lines

Returns:
A dictionary with the labels of the resource
"""
def merge(self, other: Union["Resource", None]) -> "Resource":
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Union[X, None] can be Optional[X].

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix.

def merge(self, other: Union["Resource", None]) -> "Resource":
"""Return a resource with the union of labels for both resources.

Labels that exist in the main Resource take
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
other: the resource to merge in
other: The resource to merge in.

Or lowercase and lose the period in other docstrings. Same thing for the other args/returns in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

The precedent that exists is to name both the interface and the
implementation in the sdk the same. This avoids the generally
unhelpful "Default" prefix.

opentelemetry-python is standardizing on Google style docstrings. As such
resolving formatting that is inconsistent with the style guilde.

Removing init from the abstract Resource class: it does not need to be
part of the Resource API spec. Other Resource implementations may
consume other arguments as well.
the master branch now include isort configuration that forces one import per
line.
@toumorokoshi toumorokoshi merged commit e3f8ef0 into open-telemetry:master Jul 29, 2019
This was referenced Jul 30, 2019
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* Test CircleCI build

* Test CircleCI build

* Add CircleCI Badge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants