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 Event schema #55

Closed
wants to merge 1 commit into from
Closed

add Event schema #55

wants to merge 1 commit into from

Conversation

james-rms
Copy link
Contributor

@james-rms james-rms commented Jul 31, 2022

This commit adds a schema that allows a robot to publish an Event.
An Event is intended to annotate a range of time with some metadata,
to facilitate searching the dataset. The content and field names
for this message closely mirrors the Event API exposed by Foxglove
Data Platform.

Public-Facing Changes

Description

Fixes #31

This commit adds a schema that allows a robot to publish an Event.
An Event is intended to annotate a range of time with some metadata,
to facilitate searching the dataset. The content and field names
for this message closely mirrors the Event API exposed by Foxglove
Data Platform.
Comment on lines +803 to +819
const foxglove_EventKeyValue: FoxgloveMessageSchema = {
type: "message",
name: "EventKeyValue",
description: "a single key-value pair of metadata for an event",
fields: [
{
name: "key",
type: { type: "primitive", name: "string" },
description: "key for this metadata",
},
{
name: "value",
type: { type: "primitive", name: "string" },
description: "value of the metadata",
},
],
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an identical KeyValuePair in #17. Want to remove Event from the name and "metadata" from the descriptions so we can reuse it in both places?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok for now. The Event schema might evolve more over time (e.g. to support other structured data like nested arrays / structs) but we can cross that bridge when we get to it.

const foxglove_Event: FoxgloveMessageSchema = {
type: "message",
name: "Event",
description: "a message used to annotate some event of interest in the data",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I believe we are capitalizing sentences for descriptions on the other types/fields in this file

timestamp: Time;

/** duration of event, starting at `timestamp` */
duration: Duration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should duration be optional to indicate an instantaneous event? Or do we require specifying 0 length duration?

Copy link
Contributor

@amacneil amacneil Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional imo (for schema formats that support optionals - 0 is also fine and means the same thing in other cases)

@james-rms
Copy link
Contributor Author

Closing this PR for now, will reopen once the plan for Event ingestion is more solidified.

@james-rms james-rms closed this Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add Event schema
4 participants