-
Notifications
You must be signed in to change notification settings - Fork 793
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 Event and TimedEvent #73
Add Event and TimedEvent #73
Conversation
* Represents a timed event. | ||
* A timed event is an event with a timestamp. | ||
*/ | ||
export interface TimedEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for making Event and TimedEvent distinct? Is that specified in the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not explicitly. Here's the spec that mentions them and here is the Java implementation.
A timed event has a timestamp attached to it, so to me, it makes sense to create a type for it and I think that was also the reason why Java does it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there needs to be a distinction. An event always has a time. This is a fact not only in software but also in the physical world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event
does not have a time and there is a TimedEvent
in the spec. I think that we should stick to the spec and discuss and deviation from it with the spec team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are so many things wrong with the spec that I don't think this is the best approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I am confused why there would be an event with no time, seems like a leftover from opencensus's implementation that ended up in the API.
@danielkhan The |
I missed that one. Done. I'll wait for this to be merged and modify my PR then. |
Could we do something like this:
? That would allows the events to be simple structures and the interfaces could show a relationship between the two. Could we always use |
Changed the types to extend Event to create a TimedEvent |
Actually I think I understand the rationale. If I'm not mistaken, This would not be an issue if the API was designed differently, but unfortunately that is not the case right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File naming is inconsistent with the rest of the project.
Please suggest because as far as I can see, there are still discussions and the project is inconsistent. Can we merge and sort that out with #38? |
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 10 10
Lines 152 198 +46
Branches 8 8
=====================================
+ Hits 152 198 +46
|
* chore: 0.19.0 proposal * revert global API symbol key
* chore: 0.19.0 proposal * revert global API symbol key
This adds a type for Event and TimedEvent needed for SpanData.