APP-7267 APP-7269 Fragment versions proto changes

[ehhong]

Overview

in GetFragment:

• support returning versions
• support returning tags
• support returning usage per version
• support requesting a version

[ehhong]

i think this is a breaking change, but i think it should be okay if it's resolved before the list machines using fragment epic is launched

[michaellee1019]

I think the reason we initially did a single item is that we could return a mapping of version usage in the FragmentUsage message. I don't think the Fragment Version Tech Spec prescribes the exact message but we are planning on storing it as a map based on the Fragment Usage Stats Tech Spec.

[ehhong]

oh hm, were you thinking something like this? for my understanding, is the primary advantage that it's closer to our DB storage schema?

message FragmentUsageStats {
  int32 organizations = 2;
  int32 machines = 3;
  int32 machines_in_current_org = 4;
}

message FragmentUsage {
  string fragment_id = 1;
  map<string, FragmentUsageStats> usage = 2;
}

[michaellee1019]

I think there are a few options. Mainly we have to decide what the right balance between returning overall metrics along with version level metrics. I believe we still need to return overall metrics for a few reasons, mainly that the Fragment list page will show overall metrics:

1. The way you have it in the PR code now, all metrics are versioned and the only way to get overall metrics (usage across all versions) returned is to put in a key like "all" which isn't necessarily clean.
2. What you proposed above but then we have a similar issue with needing to store "all".
3. Could do this which has fields for revisions and overall:

message FragmentUsageStats {
  int32 organizations = 2;
  int32 machines = 3;
  int32 machines_in_current_org = 4;
}

message FragmentUsage {
  string fragment_id = 1;
  FragmentUsageStats fragment_usage = 2;
  map<string, FragmentUsageStats> fragment_revision_usage = 3;
}

LMK what you think. We could also change fragment_usage to overall_usage to be more clear but the way I wrote it out is not a breaking change.

[ehhong]

ah, okay. i didn't realize that we also needed to show overall metrics. this makes sense!

[ehhong]

@michaellee1019 i updated to your suggestion. i renamed to overall_usage because i think it's a breaking change anyway to move the stats out of the original FragmentUsage message

[michaellee1019]

I think the reason we initially did a single item is that we could return a mapping of version usage in the FragmentUsage message. I don't think the Fragment Version Tech Spec prescribes the exact message but we are planning on storing it as a map based on the Fragment Usage Stats Tech Spec.

[michaellee1019]

Should these be nested in the Fragment message? Or are you doing this to only return these fields on GetFragment?

[ehhong]

good q. looking at other uses of Fragment, if i nest it in Fragment the tags will also be returned with:
GetFragmentHistory
ListFragments
CreateFragment
UpdateFragment

do we think we'll want tag info for all of these endpoints? happy to nest if so

[michaellee1019]

Yeah I think for consistency lets put into the Fragment message. The size of the data transmitted is not going to increase that much. We might not use the fields on app.viam.com in some APIs but I think it will be useful for any API customers and prevent having to call Get after List, for example.

[ehhong]

moved tags into fragment!

[mcous]

LGTM, but would like ML to sound off so we're aligned with registry language

[mcous]

@michaellee1019 we talked about this in a dev jam before the holidays: you had some thoughts on aligning our language with registry modules. What terms do we use there, and do you have any thoughts about what we should use for fragments?

I remember you mentioning "version", "tag", and "revision" as different terms

[michaellee1019]

Great point, thanks for reminding me. I think here we'd keep the terms "version" and "tag". Whenever we want to combine the two into one field, Such as A) in the robot part config or B) in the metrics storage and protos, we should use the "revision" term. We can come up with a better name too, but something like "version_or_tag" should be avoided I think.

[ehhong]

i thought version was the name for revision_or_tag?

[michaellee1019]

Oops. You are right.
Revision: The fragment is saved and given an auto-incremented revision number
Tag: A string name that is a pointer to a Revision.
Version: A reference to either a Revision number or Tag string.

[michaellee1019]

TLDR: I think the following protos use the right terminology. The rest of the changes can stay as is because the version field is either referencing a revision or a tag:

message FragmentRevision {
  string revision = 1;
  google.protobuf.Timestamp created_at = 2;
}

message FragmentTag {
  string tag = 1;
  string revision = 2;
  google.protobuf.Timestamp created_at = 3;
  google.protobuf.Timestamp updated_at = 4;
}

[michaellee1019]

I forgot this was breaking. But I think we can handle that. I think its better than keeping these fields AND having a separate object for versions