Add timestamps to aggregators and OTLP metrics exporter #1199
Conversation
ocelotl
force-pushed
the
issue_1008
branch
4 times, most recently
from
31c3d71
to
55625d7
Compare
ocelotl
changed the title
ocelotl
changed the title
| self.current = None | ||
| self.checkpoint = None | ||
| if config: | ||
| self._lock = threading.Lock() |
There was a problem hiding this comment.
Technically, all classes that extend from this would want this implementation, but design-wise, what are we deciding for having implementation within ABCs? This applies to merge and verify_type as well. See this comment.
There was a problem hiding this comment.
I think it is the right thing to have implementation in the base class if that implementation can be used by the concrete classes. There are 2 different things happening here, first, abstractmethod makes it mandatory for the concrete class to implement a specific method. Second, having implementation in the parent class. The second thing does not go against the first one, it is independent and it fulfills a separate need, the one of having common code in one single place. We can move the lock instantiation to the concrete classes if that is the project consensus, but I think that would just mean more repeated code 🤷♂️
There was a problem hiding this comment.
Yeah personally I don't mind having implementation in the base class. Just something I'd thought I'd bring up.
|
|
||
| @abc.abstractmethod | ||
| def merge(self, other): | ||
| """Combines two aggregator values.""" | ||
| self.last_update_timestamp = max( |
There was a problem hiding this comment.
Can we safely assume that all aggregators want this behaviour as the default? LastValueAggregator already doesn't follow this.
There was a problem hiding this comment.
Actually, this is not the default behavior, it is just common code that all (or most) aggregators can use if they decide so, by calling super themselves.
opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/aggregate.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/aggregate.py
Outdated
Show resolved
Hide resolved
…te.py Co-authored-by: alrex <aboten@lightstep.com>
Description
Add timestamps to aggregators and OTLP metrics exporter.
Fixes #1008
Type of change
How Has This Been Tested?
Refactored aggregator and OTLP metric exporter tests.
Checklist: