APP-7101 Add machines in org count to fragments APIs

[ehhong]

Overview

Send the number of machines in the current org that are using a fragment.

Notes

I'm a little hesitant about this approach of modifying the fragment message itself because the fragment itself has no inherent connection to a "current org". If we one day want to reuse the message Fragment outside of the context of an org, this value will be invalid / irrelevant. What do we think of another message FragmentWithOrgUsage ? This would force us to add it as a duplicate field though to avoid a breaking change. We could also consider making the machines_in_org_count value optional

message FragmentWithOrgUsage {
  Fragment fragment = 1;
  int32 machines_in_org_count = 2;
}

// or 

message FragmentWithOrgUsage {
  string fragment_id = 1;
  int32 machines_in_org_count = 2;
}

[mcous]

I'm a little hesitant about this approach of modifying the fragment message itself because the fragment itself has no inherent connection to a "current org"

I think your concern is valid, maybe we could return a separate usage field in the response? Might also give use the room to deprecate the robot_part_count and organization_count fields of Fragment at a later date

message Fragment {
  // ...
}

message FragmentUsage {
  int32 machines_in_org_count = 1;
}

message ListFragmentsResponse {
  repeated Fragment fragments = 1;
  map<string, FragmentUsage> usage_by_fragment_id = 2;
}

message GetFragmentResponse {
  Fragment fragment = 1;
  FragmentUsage usage = 2;
}

[ethanlookpotts]

Agreed with @mcous - your concern is valid, and hoisting usage to the GetFragmentResponse message level avoids coupling usage with the fragment itself.

I don't know what your anticipated usage is like, but I think there's an argument to be made that this should be a separate API to keep the usage completely decoupled from the fragment itself. Something like this:

message GetFragmentUsageRequest {
  string organization_id = 1;
}

message GetFragmentUsageResponse {
  FragmentUsage usage = 1;
}

Decoupling further would avoid any breaking changes to the existing API as well. You would need to think about whether or not you need usage in ListFragments as well.

[ehhong]

this looks good to me. should we use repeated FragmentUsage instead of a map here for additional flexibility? thinking about fragment versions, i think repeated might just be better supported in proto as well (preserved ordering, etc)

message FragmentUsage {
  int32 fragment_id = 1;
  int32 machines_in_org_count = 2;
}

message ListFragmentsResponse {
  repeated Fragment fragments = 1;
  repeated FragmentUsage usage_by_fragment_id = 2;
}

[mcous]

• 👍 repeated makes sense to me.
• Usage data is needed for all the UIs that use these endpoints, so I'd prefer to keep them in the same endpoint

[ehhong]

@ethanlookpotts fyi, we do need usage in ListFragments as well. would you still recommend creating a separate endpoint? we would have to duplicate fragment query-ing logic (visibility, by org, show public, etc) or wait for a list of fragment IDs from ListFragments

[ethanlookpotts]

Gotcha - I would personally keep the map and eventually make the fragment usage be this, but it's ultimately your call if you prefer the repeated field!

message FragmentUsage {
  map<string, int32> machines_count_by_version = 2;

  reserved 1;
  reserved 'machines_in_org_count';
}

[michaellee1019]

I crafted a proto suggestion that is similar what Ethan suggested. Having a separate API initially seems like overkill but it helps follow the request of a specific org_id and expecting the results to match what is requested. Less ambiguity with this approach, and can support 1+ fragments:

message GetFragmentsUsageRequest {
  string organization_id = 1;
  repeated string fragment_ids = 1;
}

message GetFragmentUsageResponse {
  repeated FragmentUsage usage = 1;
}

message FragmentUsage {
  string fragment_id = 1;
  int64 machines_in_org_count = 2;
}

I know that probably adds some complexity on the frontend? Having to interleave results from two APIs into one table.

[ehhong]

i think the bigger problem with this is we will have to wait for the ListFragments request to return to supply the fragment_ids to this endpoint, so fragment usage stats would display delayed

[mcous]

I agree with Emily, we need to avoid request waterfalls if we separate out the endpoints. I'm amendable to two requests if we can fire them off concurrently, but I still feel like one request is a better UX

[michaellee1019]

Ok yeah I figured this. Let's not do another API. Now I'm thinking similar to Michael's comment above which is to move and rename all usage stats from the Fragment message to the Usage message. Eventually deprecating the existing ones in the Fragment message:

message FragmentUsage {
  int64 total_organizations = 1;
  int64 total_machines = 2;
  int64 total_parts = 3;
  int64 current_organization_machines = 4;
...
}

(and then add versions on top of this as agreed to above).

[mcous]

Added @ethanlookpotts as a reviewer for proto opinions

[ehhong]

what is the full set of stats that we need? the fragment message currently reports robot_part_count but the scope designs show a "machines deployment" count?

[michaellee1019]

Sorry yeah I included extras in my example proto. I think this is the fields needed for the initial scope. The two existing fields in the existing Fragment proto are renamed and one field added:

message FragmentUsage {
  int64 total_organizations = 1; // (was organization_count)
  int64 total_machines = 2;  // (was robot_part_count)
  int64 current_organization_machines = 3;
}

[ehhong]

is total machines a different number than robot part count?

[mcous]

I think we get to decide, but I'd say "yes, let's finally do machines instead of parts." But we can bail out on that and continue to do parts if writing the logic gets too hairy

[ehhong]

updated to machines!

[ehhong]

thoughts on a better plural name for this?

[mcous]

I think this name is fine! But also I think this is the wrong response. We need usage stats in ListFragments, not ListMachine Fragments

[ehhong]

myyy bad 😅 changed to ListFragments

[michaellee1019]

This is a bit weird since in most APIs the organization_id is the owner of the entity. This is really current_organization_id so lets name as such in my opinion. And I think it makes sense to make machines_in_org be machines_in_current_org

[ehhong]

i agree, we should probably move to current_organization_id in other endpoints too to be extra clear about this. updated both fields to have "current org" in it

[mcous]

LGTM, I think this will be a pretty workable API