-
Notifications
You must be signed in to change notification settings - Fork 231
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 'log_info' definition to metadata schema #448
Conversation
- `end_time` | ||
- `start_time` (Number) - Start time for the log data | ||
- `end_time` (Number) - End time for the log data | ||
- `log_start_time` - (Number) - Start time of the log from which this was created |
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.
Do we need both start_time
and log_start_time
still? I think part of the reason we had these internally was because of the way the code was developed. Where one was supposed to represent the full duration of the log itself, and the other window you requested (echoing back what you sent to start up the connection).
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.
Correct. Do we no longer need this distinction? It seems like it is still interesting though could use a better description of why we have both.
Our use case grew to include this because we started with a batch type workflow, but once we could stream, it became desirable to explore the adjacent data as well.
Having said that, we could remove it and leave it as a custom thing. It is easy enough to spec that you can ask for more time, but you just get an error if it is no available.
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.
Another angle is that these two names are just quite confusing. It's not clear from reading them what the difference is.
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.
ok, I'll limit it to start_time
and end_time
as it's easier to add others later than remove them.
41d5de7
to
51e6c9f
Compare
a230076
to
a32ab0f
Compare
@jlisee updated to just be |
a32ab0f
to
3c50fdc
Compare
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.
Looks good but the schema needs a tweak.
@@ -0,0 +1,15 @@ | |||
{ | |||
"id": "https://xviz.org/schema/session/log_info.json", |
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.
Looks like we missing a hookup here in the session.schema.json
file for this type?
We use this data in the builder and parser so it needs defined properly.
e222142
to
8cfc7b6
Compare
We use this in the builder and parser so it needs defined properly.