Add last_updated_timestamp for observers

[lzchen]

Fixes [#510]

Refactors last_updated_timestamp into aggregators instead of bound metric instrument.

[mauriciovasquezbernal]

I wondering if we are handling timestamps correctly. According of the specification, we should only capture the timestamps once per collection interval instead of each event: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#time

Am I missing something?

[lzchen]

@mauriciovasquezbernal
Thanks for reviewing! I added a comment to the otep which asks about this concern. It seems there are some cases (Azure specifically) that we require the actual timestamp of the event, rather than jus the collection time.

[mauriciovasquezbernal]

I'm approving now. Just two minor nits. I think we should keep an eye on the fact that we are capturing the timestamps on every event.

[mauriciovasquezbernal]

LGTM, thanks for addressing all the comments.

[toumorokoshi]

I think my only comment is around the DRY of the code, but the content looks good!

If the time comparison code can be converted to a utility method, that would be great.

[toumorokoshi]

I feel like when this code shows up three times, it might be good to refactor it into a common utility method. All the timestamp comparisons seem to be doing the same logic.

[lzchen]

I agree with taking out repetitive code. Would passing an aggregator into a utility method look weird? The observer aggregator also modifies last, so it is not exactly the same; type checking would be needed. Is that logic worth it to simply reduce some duplicate code?

[toumorokoshi]

I was considered passing in two Optional[DateTime] objects, and doing the none check there
you could pass the object in itself, but I agree that feels weird

[lzchen]

I made a change that factors out the checking logic and assigns the values as well.

[toumorokoshi]

Great, thanks! If you can fix the build, I think we're good to go!