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

Report dropped attributes/events/links for otlp/jaeger/zipkin exporters #1893

Merged
merged 25 commits into from
Jun 30, 2021

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Jun 4, 2021

Description

As mentioned in issue #1850, the spec added definition for how dropped attributes, links and events counts should be reported via non-otlp exporters. This PR addresses that.

This PR has been updated with the latest changes from the refactors.

Fixes #1850

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests added

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@codeboten codeboten requested review from a team, aabmass and lzchen and removed request for a team June 4, 2021 19:12
@codeboten codeboten changed the title Codeboten/dropped attributes Report dropped attributes/events/links for jaeger/zipkin exporters Jun 4, 2021
@codeboten codeboten added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jun 4, 2021
@codeboten codeboten changed the title Report dropped attributes/events/links for jaeger/zipkin exporters Report dropped attributes/events/links for otlp/jaeger/zipkin exporters Jun 4, 2021
@codeboten codeboten requested a review from ocelotl June 7, 2021 19:06
@@ -272,6 +272,18 @@ def _translate_data(
self._translate_events(sdk_span)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe proto defines dropped_attributes_count nested inside Event and Link as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this still applies?

@codeboten codeboten requested review from ocelotl and lzchen June 9, 2021 16:07
if maxlen < 0:
raise ValueError
self.maxlen = maxlen
self._immutable = immutable
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... this does look a bit out of place, since isinstance(BoundedDict, MutableMapping) will be True.

self.maxlen = maxlen
self._immutable = immutable
self.dropped = 0
self._dict = OrderedDict() # type: OrderedDict
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up, we still support 3.6, since 3.7 dicts are insertion ordered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is code that's already existing in the SDK as BoundedDict, I moved it to the API as I needed it for Links

if maxlen is not None:
if not isinstance(maxlen, int):
raise ValueError
if maxlen < 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should be doing type checking, but if we do, this check can be collapsed with an or.

def from_map(cls, maxlen, immutable, mapping):
mapping = OrderedDict(mapping)
bounded_dict = cls(maxlen)
for key, value in mapping.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this seems to be arbitrary, I mean why should the first be kept instead of the last?

If it is impossible to not be arbitrary, this behavior should be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I agree, although this is existing behaviour I do not wish to change w/ this PR.

return self._dict.copy()

@classmethod
def from_map(cls, maxlen, immutable, mapping):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit out of place as a class method. How about passing the mapping to __init__ as an optional argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not sure why the decision was made to have a class method on the BoundedDict in the first place.

for key, value in mapping.items():
bounded_dict[key] = value
bounded_dict._immutable = immutable # pylint: disable=protected-access
return bounded_dict
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest an implementation like this one:

from collections import OrderedDict
from threading import Lock
from unittest import TestCase
from math import inf


class BoundedOrderedDict(OrderedDict):

    def __init__(self, maxlen=inf):

        self._maxlen = maxlen
        self._dropped = 0
        self._lock = Lock()

    @property
    def maxlen(self):
        return self._maxlen

    @property
    def dropped_attributes(self):
        return self._dropped

    def __setitem__(self, key, value):

        with self._lock:

            if self.maxlen == len(self):
                del self[next(iter(self.keys()))]
                self._dropped += 1

        super().__setitem__(key, value)

    def __iter__(self):
        with self._lock:
            return super().__iter__()


class BoundedOrderedDictTest(TestCase):

    def test_bounded_ordered_dict(self):

        test_dict = BoundedOrderedDict()

        test_dict[1] = 1

        assert test_dict[1] == 1

        test_dict = BoundedOrderedDict(maxlen=2)

        test_dict[0] = 1

        assert len(test_dict) == 1

        test_dict[0] = 0

        assert len(test_dict) == 1

        test_dict[1] = 1

        assert len(test_dict) == 2
        assert test_dict.dropped_attributes == 0

        test_dict[2] = 2

        assert len(test_dict) == 2
        assert test_dict.dropped_attributes == 1
        assert list(test_dict.items()) == [(1, 1), (2, 2)]

@property
def attributes(self) -> types.Attributes:
return self._attributes
_LinkBase.__init__(self, context)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think super would take care of this automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think super would work with different arguments

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, we can achieve that by changing the parent classes, something like this:

class FirstParent:
    def __init__(self, first, *args):
        print(first)
        super().__init__(*args)


class SecondParent:
    def __init__(self, second, *args):
        print(second)
        super().__init__(*args)


class Child(FirstParent, SecondParent):
    pass


Child("first", "second")
first
second

opentelemetry-api/src/opentelemetry/attributes/__init__.py Outdated Show resolved Hide resolved
return self._attributes
EventBase.__init__(self, name, timestamp)
Attributed.__init__(
self, attributes, limit=limit, immutable=True, filtered=True
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels inconsistent. We are implementing immutability in Resource by returning a copy of attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, the handling of attributes is inconsistent currently. I'm hoping this PR will help with that. I don't know which way is the better way to handle it, but I'm open to suggestions

@property
def attributes(self) -> types.Attributes:
return self._attributes
_LinkBase.__init__(self, context)
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, we can achieve that by changing the parent classes, something like this:

class FirstParent:
    def __init__(self, first, *args):
        print(first)
        super().__init__(*args)


class SecondParent:
    def __init__(self, second, *args):
        print(second)
        super().__init__(*args)


class Child(FirstParent, SecondParent):
    pass


Child("first", "second")
first
second

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
@codeboten codeboten marked this pull request as draft June 10, 2021 20:48
@codeboten
Copy link
Contributor Author

Put this PR back to draft mode, as I'll be breaking the functionality that's not specific to the exporters in separate PRs. See #1909 for the first chunk of that work.

@codeboten codeboten force-pushed the codeboten/dropped_attributes branch from 43cd873 to 22fb2f6 Compare June 25, 2021 17:06
@codeboten codeboten marked this pull request as ready for review June 25, 2021 17:07
@codeboten codeboten requested a review from ocelotl June 25, 2021 17:07
@codeboten
Copy link
Contributor Author

@lzchen @ocelotl updated the PR to account for the other changes made in #1915 and #1909

self.assertEqual(3, span.dropped_events)
self.assertEqual(2, span.events[0].attributes.dropped)
self.assertEqual(2, span.links[0].attributes.dropped)
self.assertEqual(2, span.resource.attributes.dropped)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, are dropped attributes from events, links added to total dropped count or dropped_attributes_count is more like dropped_span_attributes_count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped attributes counts are independent for events/links/spans/resources.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. My question is whether dropped attributes counts of events/links are reported for exporters?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dropped attribute counts for events/links are reported for OTLP as defined in proto. I don't think they have been defined for the other exporters.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, I noted that in this comment. I believe we should add this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them now, please take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that according to the spec, the Zipkin exporter does not provide a mechanism to export attributes for events, and support for Links is TBD.

The Jaeger exporter described in the spec suggests implementations MAY support attributes for links via logs, but doesn't require it.

@codeboten codeboten merged commit d724573 into open-telemetry:main Jun 30, 2021
@codeboten codeboten deleted the codeboten/dropped_attributes branch June 30, 2021 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support reporting of dropped attributes/events/links
4 participants