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

Add a timestamp to Event in Observation #4032

Closed
marcingrzejszczak opened this issue Aug 16, 2023 · 2 comments · Fixed by #4034
Closed

Add a timestamp to Event in Observation #4032

marcingrzejszczak opened this issue Aug 16, 2023 · 2 comments · Fixed by #4034

Comments

@marcingrzejszczak
Copy link
Contributor

sometimes we want to pass a timestamp when an event happened. Currently we don't allow that. Example - line/armeria#4980 (comment)

@jonatan-ivanov
Copy link
Member

Is the use-case getting to know about an event that happened in the past and you know when did the event happen?
Right now we only know the timestamp of current events in the present (when onEvent is called; similarly to onStart/onStop).

I think getting to know about past event can be a fair use-case, my two cents:

  • Back in the day, the first PoC of the Recording Observation API had non-instantaneous (Observation as of today) and instantaneous (Event as of today) events, both of them hold the timestamp to tell when they happened (was "more stateful" compared to what we have today), I think this is the same what this issue is for
  • We removed this capability and made it "less stateful" so that it can be more lightweight but one of the the trade-offs was this
  • I think it can be ok to introduce a timestamp on Event if there's need for it but if we do it, it will be a little different than Observation since that one does not have a timestamp and both suppose to record that something happened
  • Also the timestamp will be there even if it is not needed
  • As an alternative, I'm also thinking about introducing an Event implementation that can have a timestamp (PastEvent, EventWithTimeStamp ? :|) but adding a timestamp to the event can be also ok

@marcingrzejszczak
Copy link
Contributor Author

The example is available here line/armeria#4980 (comment) . So in Armeria they are gathering timing information about certain events happening in the system. Then they can pass it to annotate e.g. the spans. In case of an observation api we can't really use that information because the event ignores it (and spans ignore it too)

jonatan-ivanov pushed a commit that referenced this issue Sep 7, 2023
Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>

Closes gh-4032
@jonatan-ivanov jonatan-ivanov changed the title Add a timestamp to event in Observation Add a timestamp to Event in Observation Sep 7, 2023
jonatan-ivanov pushed a commit that referenced this issue Sep 7, 2023
Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>

Closes gh-4032
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants