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

RSDK-8819: FTDC file format. #4432

Merged
merged 6 commits into from
Oct 14, 2024
Merged

Conversation

dgottlieb
Copy link
Member

No description provided.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Oct 8, 2024
ftdc/ftdc.go Show resolved Hide resolved
ftdc/ftdc.go Outdated Show resolved Hide resolved
ftdc/ftdc.go Outdated Show resolved Hide resolved
ftdc/ftdc.go Show resolved Hide resolved
ftdc/ftdc.go Show resolved Hide resolved
ftdc/custom_format.go Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved

// We now have fields, e.g: ["metric1.Foo", "metric1.Bar", "metric2.Foo"]. The `mapOrder` should
// be ["metric1", "metric2"]. It's undefined behavior for a `mapOrder` metric name key to be
// split around a different metric name. E.g: ["metric1.Alpha", "metric2.Beta",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I would have parsed that the schema columns MUST have one level of nesting. Can we write that somewhere in the explanation? I would have thought that writing ["alpha", "matric1.beta", "gamma"] would be acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar where I'd like to talk offline about options. The file format doesn't require this at all. But the usage (being map[string]any) does result in this small translation. Maybe I shouldn't have put it in this "custom format" code and rather left it as part of the caller's responsibility.

Copy link
Member

Choose a reason for hiding this comment

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

cool, lets chat about this in the in person chat

ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Show resolved Hide resolved
Copy link
Member Author

@dgottlieb dgottlieb left a comment

Choose a reason for hiding this comment

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

Responding -- patch incoming....eventually

ftdc/ftdc.go Show resolved Hide resolved
ftdc/custom_format.go Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Outdated Show resolved Hide resolved

// We now have fields, e.g: ["metric1.Foo", "metric1.Bar", "metric2.Foo"]. The `mapOrder` should
// be ["metric1", "metric2"]. It's undefined behavior for a `mapOrder` metric name key to be
// split around a different metric name. E.g: ["metric1.Alpha", "metric2.Beta",
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar where I'd like to talk offline about options. The file format doesn't require this at all. But the usage (being map[string]any) does result in this small translation. Maybe I shouldn't have put it in this "custom format" code and rather left it as part of the caller's responsibility.

ftdc/custom_format.go Outdated Show resolved Hide resolved
ftdc/custom_format.go Show resolved Hide resolved
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

It feels like @nicksanford has given this a solid review, and the general concept makes sense to me. I did not give this a solid review (i.e. line-by-line). I could do that, but I would need to meet with you synchronously 😆 , @dgottlieb, to walk through it . I don't want to slow down your development, though, and trust that this is probably fine to start accessing with another, incoming PR. Will approve for now.

fieldOrder []string
}

// writeSchema writes down names for metrics in the form of a json array. All subsequent calls to
Copy link
Member

Choose a reason for hiding this comment

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

You could put some of this documentation in a doc.go file for the package. Might be nice to have a more obvious place for the description of the format.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea! Hadn't occurred to me

// - Only write metric names when things change.
// - Minimize how much space is used to write out a value that hasn't changed.
//
// Using a pseudo EBNF notation, an FTDC file is:
Copy link
Member

Choose a reason for hiding this comment

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

ENBF

Been a while since I've heard that 😆 . Nice.

ftdc/custom_format.go Outdated Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 14, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 14, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 14, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Oct 14, 2024
@dgottlieb dgottlieb merged commit 423268b into viamrobotics:main Oct 14, 2024
18 checks passed
@dgottlieb dgottlieb deleted the RSDK-8819 branch October 14, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants