-
Notifications
You must be signed in to change notification settings - Fork 660
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
Reset start_time_unix_nano
in _ViewInstrumentMatch
for aggregation creation
#4137
Conversation
That makes sense.
That I don't understand so well. What do you mean by "resetting" Also, why change Maybe we should not even have a I added a commit in this PR that does that (removing the |
3a27ee1
to
8427bc5
Compare
something like that:
Agreed. So, let's move forward with using |
start_time_unix_nano
in _ViewInstrumentMatch
start_time_unix_nano
in _ViewInstrumentMatch
for aggregation creation
afa1fcd
to
70f3245
Compare
…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>
Description
Fixes #4123
When new aggregations are created within the same _ViewInstrumentMatch (e.g.,
histogram.record
is called multiple times with different attributes), thestart_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 constantstart_time_unix_nano
across all aggregations, regardless of when they were actually created. See a failing test in my commit test to failA potential solution is to resetstart_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.