-
Notifications
You must be signed in to change notification settings - Fork 606
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
Report dropped attributes/events/links for otlp/jaeger/zipkin exporters #1893
Conversation
...xporter-jaeger-proto-grpc/src/opentelemetry/exporter/jaeger/proto/grpc/translate/__init__.py
Outdated
Show resolved
Hide resolved
...emetry-exporter-jaeger-thrift/src/opentelemetry/exporter/jaeger/thrift/translate/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-zipkin-json/tests/encoder/test_v1_json.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-zipkin-json/tests/encoder/test_v2_json.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-zipkin-json/tests/encoder/test_v2_json.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-zipkin-json/tests/encoder/test_v1_json.py
Outdated
Show resolved
Hide resolved
@@ -272,6 +272,18 @@ def _translate_data( | |||
self._translate_events(sdk_span) |
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 believe proto defines dropped_attributes_count
nested inside Event
and Link
as well?
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 think this still applies?
if maxlen < 0: | ||
raise ValueError | ||
self.maxlen = maxlen | ||
self._immutable = immutable |
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.
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 |
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.
Just a heads up, we still support 3.6, since 3.7 dicts are insertion ordered.
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.
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: |
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.
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(): |
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.
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.
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.
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): |
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.
This is a bit out of place as a class method. How about passing the mapping to __init__
as an optional argument?
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.
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 |
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 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) |
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 think super
would take care of this automatically.
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 didn't think super would work with different arguments
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 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
return self._attributes | ||
EventBase.__init__(self, name, timestamp) | ||
Attributed.__init__( | ||
self, attributes, limit=limit, immutable=True, filtered=True |
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.
This feels inconsistent. We are implementing immutability in Resource
by returning a copy of attributes
.
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 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) |
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 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
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. |
43cd873
to
22fb2f6
Compare
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) |
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.
Hmm, are dropped attributes from events, links added to total dropped count or dropped_attributes_count is more like dropped_span_attributes_count?
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.
Dropped attributes counts are independent for events/links/spans/resources.
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.
Correct. My question is whether dropped attributes counts of events/links are reported for exporters?
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.
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.
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 believe we are not doing that in OTLP exporter https://github.com/open-telemetry/opentelemetry-python/blob/main/exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py#L169.
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.
Correct, I noted that in this comment. I believe we should add this as well.
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 added them now, please take a look
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.
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.
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.
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
Does This PR Require a Contrib Repo Change?
Checklist: