New APIs to add/remove metric readers at run-time

[JP-MY]

Description

This change adds two public functions to MeterProvider that allow registering and deleting metric readers at run-time.

Fixes #4818

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

How Has This Been Tested?

• It has been tested via new unit test added to ensure correct behavior for addition and removal of metric readers at run-time.

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: JP-MY / name: Mani Yazdankhah (9bb1567)

[aabmass]

Thanks for the PR, sorry for the delay here.

[aabmass]

It's a little unfortunate to have multiple copies of the readers on these two classes. Any ideas to get around it?

[JP-MY]

unfortunately I can't think of an easy way to decouple these as meter provider heavily realies on internals of measurment consumer from what I can see. I agree it'd be better to simplify and happy to implement changes if you have an idea on what should be done.

[aabmass]

This code currently assumes that self._reader_storages is immutable. The goal was to prevent having a single MeterProvider wide lock for all metrics.

Can you help me understand the consequences of this variable being mutable now? E.g. dropped measurements, safety of iterating over values() in event another thread modifies the dict, etc

[JP-MY]

in terms of thread safety: because of GIL (I'm aware it is becoming optional and possibly removed in the future, lets ignore that for now) all built in types are thread safe so the dict can't be pushed to / popped when iterating over values(). The only edge cases are when you call add / remove while this iteration is happening meaning you get one last set of extra data if you were trying to remove and you don't get the first batch of data if you were trying to add which should be fine as we did not provide any real time gurantees for these operations.

P.S. please correct me if I'm wrong but that is my understanding

[aabmass]

I don't think that is thread safe actually, you probably need to copy the keys or values into a list which is atomic operation with GIL held

>>> d = {"hello": "world"}
>>> v = d.values()
>>> v
dict_values(['world'])
>>> iter(v)
<dict_valueiterator object at 0x7f52311f13a0>
>>> i = iter(v)
>>> d["bar"] = "bar"
>>> next(i)
Traceback (most recent call last):
  File "<python-input-14>", line 1, in <module>
    next(i)
    ~~~~^^^
RuntimeError: dictionary changed size during iteration

So besides the mechanical issues, I just wanted to make sure you've thought about these problems.

[JP-MY]

I ran a small test and you are correct, I can get it to raise runtime error, so will have to add a lock around this as well (or do you prefer the copy to a list variation) to avoid concurrency issues. We have indeed thought about this but losing some data at the very start of adding/ very end of removing the metric reader is acceptable for our use case.

Back on the concurency issue: I prefer using a lock as it avoids extra overhead of copying data and adding GC pressure but please let me know which approach I should use.

[herin049]

We'd probably want to use a lock here, and then create a copy of the values as a WeakSet with the lock held. We probably don't want to have the lock held during this entire process since there might be instances where consume_measurement could block (not sure on this one).

[JP-MY]

Tested to make sure this approach works and updated the PR accordingly

[JP-MY]

I'm not sure why the lint is failing for protected access, I can see the same protected access in the same file. Should I add a pylint: disable= rule or is there a config for this that I need to update? also the contrib test fails don't seem related (failed to get a specific commit?) but I'm not sure if I messed something up to cause that or not