Add top level display field to structured output

[EverlastingBugstopper]

While reviewing #676, @prasek suggested that we add a top level display object to our --output=json so that folks can write wrappers around Rover for their own logging/metrics integrations.

Inital proposal from @prasek:

{
  "display": {
     "message": "Subgraph inventory published successfully to supergraph-router@dev",
     "details": [
      {
        "message": "Publishing SDL to supergraph-router:dev (subgraph: inventory) using credentials from the default profile."
      },
      {
        "message": "The 'inventory' subgraph for the 'supergraph-router' graph was updated"
      },
      {
        "message": "The gateway for the 'supergraph-router' graph was NOT updated with a new schema"
      }
    ]
  },
  "error": null
}

---

The main thing that's complicated here is how we want to handle output that... doesn't really match up super nicely with the display.details[x].message format.

For instance, if we wanted to add a new message for each new line, then a (fairly small) rover subgraph check JSON might look like this:

{
  "display": {
    "message": "The proposed schema does not break any existing client operations.",
    "messages": [
      {
   	  	"message": "Checking the proposed schema for subgraph products against rover-supergraph-demo@test"
      },
      {
	    "message": "Check Result:"
      },
	  {
	    "message": ""
      },
      {
	    "message": "Compared 3 schema changes against 0 operations"
      },
      {
	    "message": "┌────────┬─────────────┬───────────────────────────────────────────┐"
      },
      {
	    "message": "│ Change │    Code     │                Description                │"
      },
      {
	    "message": "├────────┼─────────────┼───────────────────────────────────────────┤"
      },
      {
	    "message": "│ PASS   │ FIELD_ADDED │ type `Product`: field `new_field` added   │"
      },
      {
	    "message": "├────────┼─────────────┼───────────────────────────────────────────┤"
      },
      {
	    "message": "│ PASS   │ FIELD_ADDED │ type `Product`: field `other_field` added │"
      },
      {
	    "message": "├────────┼─────────────┼───────────────────────────────────────────┤"
      },
      {
	    "message": "│ PASS   │ FIELD_ADDED │ type `Product`: field `big_field` added   │"
      },
      {
	    "message": "└────────┴─────────────┴───────────────────────────────────────────┘"
      },
      {
	    "message": "View full details at https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/1b29c4b4-219f-4c4f-97fb-07edf8a482d3?variant=test"
      }
    ]
  },
  "data": {
    "target_url": "https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/45a8991d-8841-43c2-ab1b-cd5daf0b45e4?variant=test",
    "operation_check_count": 0,
    "changes": [
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `new_field` added",
        "severity": "PASS"
      },
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `other_field` added",
        "severity": "PASS"
      },
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `big_field` added",
        "severity": "PASS"
      }
    ],
    "failure_count": 0,
    "success": true
  },
  "error": null
}

As you can see, this really balloons the size of the JSON output and also just... kinda looks strange with the way the table is formatted.

Additionally, not every command currently has a top level "message" that it prints for success (though maybe they should!) For instance, in the above JSON example, .display.message is equal to The proposed schema does not break any existing client operations., which is not currently displayed in Rover.

I wonder if it would be easier to just have the structure be .display.messages. Or maybe even easier we could just have a top level "display" that is just a string containing all of the output for that command, and just have them separated by \n.

Also, something important to note here is that some messages are simply progress messages and are not printed to stdout. Should those be included in the JSON output here? Should stdout messages be differentiated from stderr in the structured output?

[EverlastingBugstopper]

I guess another alternative is to sorta logically group the output messages, so the checks table could be its own "message" like so:

{
  "display": {
    "message": "The proposed schema does not break any existing client operations.",
    "messages": [
      {
   	  	"message": "Checking the proposed schema for subgraph products against rover-supergraph-demo@test"
      },
      {
	    "message": "Check Result:"
      },
	  {
	    "message": ""
      },
      {
	    "message": "Compared 3 schema changes against 0 operations"
      },
      {
	    "message": "┌────────┬─────────────┬───────────────────────────────────────────┐\n│ Change │    Code     │                Description                │\n├────────┼─────────────┼───────────────────────────────────────────┤\n│ PASS   │ FIELD_ADDED │ type `Product`: field `new_field` added   │\n├────────┼─────────────┼───────────────────────────────────────────┤\n│ PASS   │ FIELD_ADDED │ type `Product`: field `other_field` added │\n├────────┼─────────────┼───────────────────────────────────────────┤\n│ PASS   │ FIELD_ADDED │ type `Product`: field `big_field` added   │\n└────────┴─────────────┴───────────────────────────────────────────┘"
      },
      {
	    "message": "View full details at https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/1b29c4b4-219f-4c4f-97fb-07edf8a482d3?variant=test"
      }
    ]
  },
  "data": {
    "target_url": "https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/45a8991d-8841-43c2-ab1b-cd5daf0b45e4?variant=test",
    "operation_check_count": 0,
    "changes": [
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `new_field` added",
        "severity": "PASS"
      },
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `other_field` added",
        "severity": "PASS"
      },
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `big_field` added",
        "severity": "PASS"
      }
    ],
    "failure_count": 0,
    "success": true
  },
  "error": null
}

[prasek]

@EverlastingBugstopper thanks for these different options. Perhaps a simplified message output without the table formatting for details would work, such that it could be used in a bullet list, table or whatever output format the integration or CI pipeline wanted to use?

[EverlastingBugstopper]

Hmmmm... I guess it's kinda hard to present things as a message like that with no formatting unless you start adding some structure back in since each line is printed as a table... at which point it'd be easier to just refer people to data.changes where they can construct their own bullets/table format.

Unless I'm missing something? Just having a hard time envisioning what the simplified message output would look like here.

[prasek]

For example:

{
  "display": {
    "message": "[PASS] 3 schema changes for rover-supergraph-demo@test does not break for 0 operations. See details at https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/1b29c4b4-219f-4c4f-97fb-07edf8a482d3?variant=test",
    "details": [
      {
	    "message": "[PASS] type `Product`: field `new_field` added"
      },
      {
	    "message": "[PASS] type `Product`: field `other_field` added"
      },
      {
	    "message": "[PASS] type `Product`: field `big_field` added"
      },
    ]
  },
  "data": {
    "target_url": "https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/45a8991d-8841-43c2-ab1b-cd5daf0b45e4?variant=test",
    "operation_check_count": 0,
    "changes": [
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `new_field` added",
        "severity": "PASS"
      },
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `other_field` added",
        "severity": "PASS"
      },
      {
        "code": "FIELD_ADDED",
        "description": "type `Product`: field `big_field` added",
        "severity": "PASS"
      }
    ],
    "failure_count": 0,
    "success": true
  },
  "error": null
}

[EverlastingBugstopper]

Ah! That looks super nice!

I wonder if we should update our regular output to look more like this as well so we don't have to special case it. I've never been the hugest fan of the ASCII tables that we output anyways. Thoughts?

[prasek]

Ah! That looks super nice!

🙂

I wonder if we should update our regular output to look more like this as well so we don't have to special case it. I've never been the hugest fan of the ASCII tables that we output anyways. Thoughts?

Sounds good 👍

The display.details content above could be rendered in a table or list depending on view preference

[PASS] 3 schema changes for rover-supergraph-demo@test does not break for 0 operations. See details at https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/1b29c4b4-219f-4c4f-97fb-07edf8a482d3?variant=test
 - [PASS] type `Product`: field `new_field` added
 - [PASS] type `Product`: field `other_field` added
 - [PASS] type `Product`: field `big_field` added

[PASS] 3 schema changes for rover-supergraph-demo@test does not break for 0 operations. See details at https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/1b29c4b4-219f-4c4f-97fb-07edf8a482d3?variant=test

Details:
--------------------------------------------------------
| [PASS] type `Product`: field `new_field` added
--------------------------------------------------------
| [PASS] type `Product`: field `other_field` added.
--------------------------------------------------------
| [PASS] type `Product`: field `big_field` added
--------------------------------------------------------

[queerviolet]

in js-land, we have console.log and console.table. you could imagine a display array which basically serializes the values passed to those:

"display": [{
    "message": "[PASS] 3 schema changes for rover-supergraph-demo@test does not break for 0 operations. See details at https://studio.apollographql.com/graph/rover-supergraph-demo/operationsCheck/1b29c4b4-219f-4c4f-97fb-07edf8a482d3?variant=test",
    "details": [{
           "table": [{
              "change": "PASS",
              "code": "FIELD_ADDED",
              "description": "type `Product`: field `new_field` added"
            },
            {
              "change": "PASS",
              "code": "FIELD_ADDED",
              "description": "type `Product`: field `other_field` added"
            },
            {
              "change": "PASS",
              "code": "FIELD_ADDED",
              "description": "type `Product`: field `big_field` added"
            }]
      }]	
}],

obviously in rover-land we don't have .log or .table yet, but you imagine implementing them with an interface like this:

// i do not think we should call this `Console` btw. i suspect we already have an
// abstraction over output?
fn do_something(console: &mut Console) {
  console.log("just insert a message");
  let data: Vec<SomethingSerializable> = get_something_serializable();
  console.table(&data);
  console.log("this message provides additional detils", |details: &mut Console| {
    details.log("here are some details:");
    let details_data: Vec<_> = get_some_other_data();
    details.table(&details_data);
  });
}

i believe that we could make .table work in a fairly straightforward way by implementing a custom Serializer (which either prints a table or emits a json array).

a perhaps nicer interface would be to provide macros that take format args:

fn do_something() {
  log!("just insert a message");
  // using a macro lets us do format string substitutions.
  // - in the standard output, these will be converted to strings like format! does
  // - in json output, the parts and values could be serialized independently,
  //    which would let studio and other consumers provide a nicer devtools-like experience for viewing
  //    large lists, digging into objects, etc.
  // - this depends on only logging data items which implement both `Display` and `Serialize`, but that seems not-terrible.
  log!("here are some numbers: {}. whew, that was a lot of numbers.", vec![1, 2, 3]);
  /*
  this might serialize as:
    {
      message: "here are some numbers: [1, 2, 3]. whew, that was a lot of numbers.",
      parts: [
        {log: "here are some numbers: "},
        {data: [1, 2, 3]},
        {log: ". whew, that was a lot of numbers."}
      ]
    }
  we could also include additional type information for {data:} items, if studio or other consumers could benefit from it.
  */
  let data: Vec<SomethingSerializable> = get_something_serializable();
  table!(&data)
  // with macros, we expect the `&mut Console` to be a thread-local variable,
  // so there's no need to pass another Console to the details callback
  log!("this message provides additional details" ; || {
    log!("here are some details:");
    let details_data: Vec<_> = get_some_other_data();
    table!(&details_data)
  });
}

[queerviolet]

update: i looked at this a bit last night and it looks like tracing basically gives us these macros already, and we're using it to print our log messages already, so i'd update my suggestion to say that we should just make tracing::Subscribers for console logging and structured output logging

(the details approach above isn't really needed, bc spans / child-span / events already represent a reasonable hierarchy for logging messages)

[EverlastingBugstopper]

Unfortunately, we don't use tracing for all of our log messages as it was requested early on that we remove the INFO prefix from our progress messages and I just... for the life of me I couldn't figure out how to modify the tracing subscriber to do that so they were switched to eprintln or println statements (unless they are in a subcrate). All tracing level logs are suppressed unless you specify a --log level.

Due to the large nature of the changes necessary here, the original requirements for structured output were to match that of the apollo CLI (which did not have a display field), the ever-growing list of issues, and my limited time, this feature will be postponed.

We have structured output that allows you to construct reasonable messages/programs based on strong types and if you want to display what Rover usually displays, you can run with --output plain or just leave off the optional argument.

We won't close this issue as it's something we still want to do, we just don't have the bandwidth for it right now.

[EverlastingBugstopper]

Closing this as won't do - nobody external has asked for this feature.