Added set_status to span, updated pymongo integration

[victoraugustolls]

• Added set_status to spans, according to specification

• Added set_status_to_current_span to Tracer

• Swapped pymongo integration attributes to specs described here

Added tests do maintain 100% coverage

[victoraugustolls]

Should we update span to initiate with OK status? Reference here

If used, this will override the default Span status, which is OK.

[reyang]

LGTM. Feel free to continue work on the default value for Status class, and let me know when you're ready to merge it.

[victoraugustolls]

LGTM. Feel free to continue work on the default value for Status class, and let me know when you're ready to merge it.

Would it be okay to rewrite the Status class as this and fix the tests:

# Copyright 2017, OpenCensus 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 google.rpc import code_pb2


class Status(object):
    """The Status type defines a logical error model that is suitable for
    different programming environments, including REST APIs and RPC APIs.
    It is used by gRPC.

    :type code: int
    :param code: An enum value of :class: `~google.rpc.Code`.

    :type message: str
    :param message: A developer-facing error message, should be in English.

    :type details: list
    :param details: A list of messages that carry the error details.
                    There is a common set of message types for APIs to use.
                    e.g. [
                            {
                                "@type": string,
                                field1: ...,
                                ...
                            },
                         ]
                    See: https://cloud.google.com/trace/docs/reference/v2/
                         rest/v2/Status#FIELDS.details
    """
    def __init__(self, code, message=None, details=None):
        self._code = code
        self._message = message
        self.details = details

    @property
    def canonical_code(self):
        return self._code

    @property
    def description(self):
        return self._message

    @property
    def is_ok(self):
        return self._code == code_pb2.OK

    def format_status_json(self):
        """Convert a Status object to json format."""
        status_json = {}

        status_json['code'] = self._code

        if self._message is not None:
            status_json['message'] = self._message

        if self.details is not None:
            status_json['details'] = self.details

        return status_json

    @classmethod
    def from_exception(cls, exc):
        return cls(
            code=code_pb2.UNKNOWN,
            message=str(exc)
        )

    @classmethod
    def as_ok(cls):
        return cls(
            code=code_pb2.OK,
        )

This is a lot closer to the specification.

If it isn't ok because it is a breaking change making code and message private, we can leave them public until the migration, and then we just rename them in the future with the _ before.

[victoraugustolls]

@reyang updated Status as described above, if you want do discuss it. I think the Status is a lot closer to spec, but maybe maintaining code and message as before, might be better for now. Let me know what you think.

[reyang]

Would it be okay to rewrite the Status class as this and fix the tests:
...
This is a lot closer to the specification.
If it isn't ok because it is a breaking change making code and message private, we can leave them public until the migration, and then we just rename them in the future with the _ before.

It looks good to me! I don't know if the SDK consumers would use the status class directly. What they would do is to set status, and this is the place we might break them.

Given we're still in beta, I guess it is probably okay to have breaking changes for a small set of customers? @c24t for more input.

[c24t]

Added set_status to spans, according to specification

The change may be ok, but in general OpenCensus does not follow the OpenTelemetry spec. This isn't described in the OC spec.

[c24t]

The code and tests look great, but renaming Status.code and Status.message is a breaking change for no obvious benefit. LGTM otherwise.

[c24t]

We shouldn't link to the OT spec here.

[reyang]

I have slightly different opinion on this.
I agree that OpenCensus Python should follow OpenCensus spec. Following OpenTelemetry spec is not the goal here, but it is nice to have if:

1. It doesn't go against OpenCensus spec.
2. It doesn't introduce major breaking changes (given we're moving to OpenTelemetry soon) without a good rationale.
3. It helps us to form better understanding on our path to OpenTelemetry, or simply make the job easier when we try to steal some code and morph them into OpenTelemetry.

With these, I think having a link to OT spec is similar like saying "we are supporting W3C CorrelationContext version 2" - not something required in OC spec, but a nice to have feature.

[c24t]

I agree with all that, I'm just concerned about putting a link to the OT spec in something as public as the release notes. There's already a lot of confusion around OC/OT, linking the OT spec here might invite more.

[c24t]

The changes to add set_status look good, but this looks like a breaking change for the sake of matching the names in a different spec.

[victoraugustolls]

@c24t thanks for the review! Just some clarifications:

The change may be ok, but in general OpenCensus does not follow the OpenTelemetry spec. This isn't described in the OC spec.

My idea was to don't do anything that breaks the OpenCensus Specs and change some internals to make an easier transition in the future (OpenTelemetry).

The code and tests look great, but renaming Status.code and Status.message is a breaking change for no obvious benefit. LGTM otherwise.

Is it okay to maintain the @property's and the original code and message names? This does not breaks the OC specs and also make future transitions easier / OT compatible.

[victoraugustolls]

@reyang @c24t Is there anything pending here? Would be interesting to merge before releasing 0.7.0!

[reyang]

@victoraugustolls I'm fine with the change, and I've signed off.
@c24t would like to get your feedback before I merge this.

BTW, @victoraugustolls I've sent some messages to you on gitter, would you take a look? Thanks!

[reyang]

@victoraugustolls we already have 0.7.0 release done.

We can consider a minor release (e.g. 0.7.1) if there is no breaking change.

[c24t]

Is it okay to maintain the @property's and the original code and message names? This does not breaks the OC specs and also make future transitions easier / OT compatible.

That sound great to me. Even though it's a minor change and doesn't technically run afoul of OC spec I think it's better to keep the existing names.

[victoraugustolls]

That sound great to me. Even though it's a minor change and doesn't technically run afoul of OC spec I think it's better to keep the existing names.

Done, also I updated pymongo changelog, if you could take a look. Already updated with master. If I need to do anything else, please let me know!

[c24t]

Looks great, thanks @victoraugustolls!