Skip to content
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

Reset start_time_unix_nano in _ViewInstrumentMatch for aggregation creation #4137

Merged
merged 5 commits into from
Aug 21, 2024

Conversation

emdneto
Copy link
Member

@emdneto emdneto commented Aug 20, 2024

Description

Fixes #4123

When new aggregations are created within the same _ViewInstrumentMatch (e.g., histogram.record is called multiple times with different attributes), the start_time_unix_nano remains constant, which seems incorrect.

The current implementation of _ViewInstrumentMatch reuses the initial timestamp from when the instrument match was created, even for new aggregations with different attributes.

For example, suppose I record a measurement now (e.g., histogram. record(1, {"foo": "bar"})) and later record the same histogram with different attributes (e.g., histogram.record(1, {"foo": "bar1"})). In that case, the SDK creates a new aggregation. However, it incorrectly assigns the initial start_time_unix_nano from when _ViewInstrumentMatch was initialized rather than reflecting the actual time when the new aggregation was created. This results in a constant start_time_unix_nano across all aggregations, regardless of when they were actually created. See a failing test in my commit test to fail

A potential solution is to reset start_time_unix_nano each time the collect method is called, ensuring that new aggregations are associated with a recent timestamp. I'll likely discuss this approach further.

@ocelotl suggested a better solution to just use time_ns() during aggregation creation, which also fixes the issue.

@emdneto emdneto added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 20, 2024
@emdneto emdneto requested a review from a team August 20, 2024 14:42
@emdneto emdneto marked this pull request as draft August 20, 2024 14:45
@ocelotl
Copy link
Contributor

ocelotl commented Aug 20, 2024

When new aggregations are created within the same _ViewInstrumentMatch (e.g., histogram.record is called multiple times with different attributes), the start_time_unix_nano remains constant, which seems incorrect.

That makes sense.

A potential solution is to reset start_time_unix_nano each time the collect method is called, ensuring that new aggregations are associated with a recent timestamp. I'll likely discuss this approach further.

That I don't understand so well.

What do you mean by "resetting" start_time_unix_nano each time collect is called? You mean setting start_time_unix_nano to a constant value? Which value would it be?

Also, why change start_time_unix_nano every time collect is called? You mean the _start_time_unix_nano attribute in the _ViewInstrumentMatch object? Aggregations that use a CUMULATIVE collection aggregation temporality need this value to be the same every time collect is called.

Maybe we should not even have a _start_time_unix_nano attribute in the _ViewInstrumentMatch object at all. That attribute should only belong to the aggregation object and should be set to the moment the aggregation object is created.

I added a commit in this PR that does that (removing the _start_time_unix_nano attribute from _ViewInstrumentMatch and setting the _start_time_unix_nano attribute in the aggregation object to the moment the aggregation is created. I think that approach fixes this issue and does not affect the collect method of the aggregations when they use CUMULATIVE as the collection aggregation temporality.

@ocelotl ocelotl force-pushed the view-instrument-match-fix branch 2 times, most recently from 3a27ee1 to 8427bc5 Compare August 20, 2024 23:52
@ocelotl ocelotl marked this pull request as ready for review August 20, 2024 23:53
@emdneto
Copy link
Member Author

emdneto commented Aug 21, 2024

@ocelotl

What do you mean by "resetting" start_time_unix_nano each time collect is called? You mean setting start_time_unix_nano to a constant value? Which value would it be?

something like that: self._start_time_unix_nano = collection_start_nanos in the collect method.

Maybe we should not even have a _start_time_unix_nano attribute in the _ViewInstrumentMatch object at all. That attribute should only belong to the aggregation object and should be set to the moment the aggregation object is created.

Aggregations that use a CUMULATIVE collection aggregation temporality need this value to be the same every time collect is called.

Agreed. So, let's move forward with using time_ns() in each new _create_aggregation call.

@emdneto emdneto removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 21, 2024
@emdneto emdneto changed the title reset start_time_unix_nano in _ViewInstrumentMatch Reset start_time_unix_nano in _ViewInstrumentMatch for aggregation creation Aug 21, 2024
@emdneto emdneto added metrics sdk Affects the SDK package. labels Aug 21, 2024
@ocelotl ocelotl force-pushed the view-instrument-match-fix branch from afa1fcd to 70f3245 Compare August 21, 2024 19:31
@ocelotl ocelotl merged commit 909708c into open-telemetry:main Aug 21, 2024
376 checks passed
@lzchen lzchen mentioned this pull request Aug 26, 2024
9 tasks
hyoinandout pushed a commit to hyoinandout/opentelemetry-python that referenced this pull request Aug 29, 2024
…n creation (open-telemetry#4137)

* test to fail

* Use current time when creating an aggregation in _ViewInstrumentMatch

* add changelog and improve test

* fix changelog

* fix test

Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>

---------

Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Histogram.record() producing inaccurate start_time_unix_nano value
3 participants