Expose hint levels #200
Expose hint levels #200
Conversation
Composition hints in federation 2 are generated with a "level" (which can currently be one of of "DEBUG", "INFO" or "WARN"). This commit modify harmonizer to include those levels in the output and the type definitions to decode them, and include them in the `supergraph` output for future use by `rover`. Fixes apollographql#102
| } | ||
| let maybe_captures = RE.captures(&self.message); | ||
| if let Some(captures) = maybe_captures { | ||
| (captures.get(1).unwrap().as_str().to_string(), captures.get(2).unwrap().as_str().to_string()) |
There was a problem hiding this comment.
is there a more structured way that we can get this data from the underlying JS library? I don't love the need to use a regex here for this - if we could instead put that info in a JSON object or something instead of parsing out the string I think this will be more robust
There was a problem hiding this comment.
I'm not a huge fan either, but I was worried about either making it weird to use, or have backward compatibility problems.
Let me however remark that this method is not about the hint "level". A hint has fundamentally 3 components at the moment:
- the
message, which describe the particular hint. - the
code, which categorize hints (same as we have for errors). - the
level, which isWARN,INFOorDEBUGfor now.
The level, that is introduced by this patch, is shipped as a separate field in the underlying JSON object, and thus deserialized separately, so no worry on this one.
But what this method is trying to do is give access to the message and code separately. Because the issue is that unfortunately, instead of having the message and code as separate fields in the underlying JSON, do_compose.js has been using the Hint.toString() method to generate a single pre-formatted string of the form "[<code>] <message>" and calling this "message".
So first, I acknowledge that this isn't strictly hint level related and so one could make the argument this shouldn't be in this patch. But I think the ultimate goal should be that in the rover UX, errors and hints can be displayed in a consistent way, where errors can just be considered as the highest "hint level" (in fact, we do hope to "merge" hints and errors in the code at some point in the future to clean things up). But doing that cleanly implies rover can access the hint code and message separately, the same way it does for errors, so it can maybe colorize the code and/or format it however it pleases. This could also allow flexibility like grouping messages by code for both hints and errors, or filtering certain code, etc.
Long story short, the current BuildHint.message field of hint.rs is currently unfortunately not really just the hint message, it is a pre-formatted string with both the code and (raw) message and this method is meant to provide a way out of this.
In theory, a better alternative would be to change BuildHint.message to only be the actual hint message and to add a BuildHint.code for the code, and to maybe also ensure those are separate in the underlying JSON object so it's easier to do, but I was kind of worried about breaking things by doing those things. So the idea of this method was to keep BuiltHint.message unchanged so we don't break existing usages, but to allow future usages to be able to access the code and message components individually.
Anyway, hope this clarify. If you're not worry about the concerns around breaking existing code, I'm happy to change all this, but I think it would require some care around versioning this properly, because if we change this, currently rover version would need to not use federation-rs versions with such changes (or rather, if they do, the hint will suddenly not be displayed with their code since it wouldn't be part of BuildHint.message anymore).
There was a problem hiding this comment.
I don't think we have to worry about breaking existing code! We should definitely change the underlying library code away from the .toString here - the only thing that matters is that we don't remove fields from the JSON that's actually emitted by the supergraph binary. Everything else is an internal contract - we only need to hold up the end to end guarantee.
There was a problem hiding this comment.
the only thing that matters is that we don't remove fields from the JSON that's actually emitted by the
supergraphbinary
But currently, the JSON of the supergraph binary has the message field for hints that is the pre-formatted string containing both the "raw" message and the code. And existing rover versions kind of assume this is what this field contains, but we ultimately want rover to be able to access separately and not as a pre-formatted string. So if we do the "cleanest" change here without backward compatibility concerns, we would be changing that message field to now be only the "raw" message, without code, and the code would be a new field. Technically this is not a field removal, but it's changing what the field holds, and I assume that would be an issue too?
The alternative could be keep the message field containing the pre-formatted string (we can still stop having the pre-formatted string in the JSON from javascript, but the supergraph binary itself would be the one formatting the string for its own JSON output), but also add 2 new fields, code and raw_message, with the message field essentially becoming deprecated but kept for backward compatibility.
Does that make sense, or am I overcomplicating this?
There was a problem hiding this comment.
Technically this is not a field removal, but it's changing what the field holds, and I assume that would be an issue too?
Yeah - I think you're right here, we want older versions to still be able to work.
The alternative could be keep the message field containing the pre-formatted string (we can still stop having the pre-formatted string in the JSON from javascript, but the supergraph binary itself would be the one formatting the string for its own JSON output), but also add 2 new fields, code and raw_message, with the message field essentially becoming deprecated but kept for backward compatibility.
Old versions of Rover will discard any new unused JSON fields (we should test this to verify of course), but this approach sounds perfectly reasonable to me!
There was a problem hiding this comment.
Thanks for the feedback. I'll try to update the patch accordingly ... when I find a bit of time for it.
| hints.push({ message: composed_hint.toString() }); | ||
| hints.push({ | ||
| message: composed_hint.toString(), | ||
| level: composed_hint.definition.level, |
There was a problem hiding this comment.
it looks like we are putting that info right into the JSON object here in the JS code - can we just do something like
let data = serde_json::from_string(data_from_js);
let message = data["message"];
let level = data["level"];?
Composition hints in federation 2 are generated with a "level" (which can currently be one of of "DEBUG", "INFO" or "WARN"). This commit modify harmonizer to include those levels in the output and the type definitions to decode them, and include them in the
supergraphoutput for future use byrover.Fixes #102