Fix/copy measurement attributes #4627
Conversation
|
|
P4rk
force-pushed
the
fix/copy-measurement-attributes
branch
from
582958e to
b48902f
Compare
P4rk
force-pushed
the
fix/copy-measurement-attributes
branch
from
b48902f to
f945980
Compare
P4rk
force-pushed
the
fix/copy-measurement-attributes
branch
from
6baf134 to
857165f
Compare
There was a problem hiding this comment.
Thanks again for the PR! The biggest possible impact I imagine is a performance regression, but it may be minimal for small attribute sets. Could you run the SDK benchmarks tox -e benchmark-opentelemetry-sdk on your machine before and after and see if there is any significant change? Admittedly we don't have great benchmark coverage here so feel free to add some new benchmark cases if you see fit
| # one will come from napoleon extension and the other from autodoc extension. This | ||
| # will raise an sphinx error of duplicated object description | ||
| # See https://github.com/sphinx-doc/sphinx/issues/8664 |
There was a problem hiding this comment.
It was. Having the lines below the TODO indented like this means that PyCharm treats them as the "same" TODO comment, rather than multiple different comments.
I have removed this as it appears to be specific to PyCharm.
| super().__setattr__( | ||
| "attributes", | ||
| deepcopy(self.attributes), | ||
| ) |
There was a problem hiding this comment.
Is this to work around frozen=True?
There was a problem hiding this comment.
Yes, using the object's __setattr__ allows us to do this and avoid the frozenness of the dataclass.
|
|
||
| @pytest.fixture | ||
| def unix_time() -> int: | ||
| return time_ns() |
There was a problem hiding this comment.
Can you hardcode a timestamp here just for repeatability?
There was a problem hiding this comment.
I have replaced this with a hardcoded timestamp
xrmx
moved this from Ready for review
to Reviewed PRs that need fixes
in @xrmx's Python PR digest
Set the attributes on the measurement as a copy of the attributes passed in. This makes sure that they cannot be changed, by using a reference from outside the scope of the measurement instance.
P4rk
force-pushed
the
fix/copy-measurement-attributes
branch
from
857165f to
36bc464
Compare
|
@aabmass Sorry I haven't got back to you in a while. Life happened. I have run |
2 participants
Description
This change gets the Measurement dataclass to create its own copy of the attributes that are passed to it. This stops changes to the attributes from outside the scope of the measurement affecting it.
I had considered making the Measurement attributes immutable, but that has a larger impact on the rest of the codebase and does more than just fix #4610.
Fixes #4610
Type of change
How Has This Been Tested?
Two new test files were added: a unit test and an integration test.
opentelemetry-sdk/tests/metrics/integration_test/test_data_point_creation.py::test_measurement_collectionopentelemetry-sdk/tests/metrics/test_measurement.py::test_measurement_attribute_is_a_different_objectopentelemetry-sdk/tests/metrics/test_measurement.py::test_measurement_attribute_uneffected_by_changeThese are new files and pytest tests. There were a few
pytestorunittesttests in the project. Let me know if these tests would fit better in a different file, or asunittesttests.Does This PR Require a Contrib Repo Change?
Checklist: