Restructure AIP-193 #1426

jskeet · 2024-09-26T14:07:35Z

No description provided.

aip/general/0193.md

noahdietz · 2024-09-26T20:22:52Z

aip/general/0193.md

+The `message` field is a a developer-facing, human-readable "debug message"
+which **should** be in English. (Localized messages are expressed using
+a `LocalizedMessage` within the `details` field.) Any dynamic aspects of
+the message **must** be included as metadata within the `ErrorInfo` message


Suggested change

the message **must** be included as metadata within the `ErrorInfo` message

the `message` **must** be included as metadata within the `ErrorInfo` message

Nit: consistently format as code when referring to the field?

Let's talk about this a bit more - I tend to find that it's worth keeping things not in code font when they're not explicitly talking about the field (which this isn't - it's talking about the semantic meaning). I'm happy to be persuaded, and I suspect there will be some areas where I'm inconsistent already... but to my eyes, a page where a huge proportion of the text is in code font ends up being hard to read.

I agree with your position 100%. I think I was getting confused with semantic meaning vs. concrete reference reading the document top to bottom.

That said, my personal tendency is to explicitly state "message value" or "the value of message" or "the content of message" rather than depend on context clues (like code formatting) to do that "comprehension lifting". I'm not too fussed about it though.

Agreed. I'd only put the word "message" in quotes when you're referring directly to the field.

aip/general/0193.md

noahdietz · 2024-09-26T21:31:52Z

aip/general/0193.md

+This practice is critical so that machine actors do not need to parse error
+messages to extract information.


Nit: similar to earlier, this reasoning would be best in a Rationale section but I'm not going to be picky

Let's leave this open so we can move it and link.

I haven't found a good way of doing this. I'm tempted to suggest we get this PR in but raise an issue to review the new text carefully for rationale-like text - I suspect there's a fair amount of it here. Personally I'm not hugely against it in this particular case, mind you...

That's fine with me (punting)

aip/general/0193.md

noahdietz · 2024-09-26T21:33:42Z

aip/general/0193.md

+- "zone": "us-east1-a",
+- "vmType": "e2-medium",
+- "attachment": "local-ssd=3,nvidia-t4=2",
+- "zonesWithCapacity": "us-central1-f,us-central1-c"


Should these be formatted as code? I guess wondering why it was switched to bulleted list even though it is a "code examle"?

I'm not sure... it's not really code - they're name/value pairs. (I replaced the example entirely, as I thought it was unrealistic - if we're going to use books, I wouldn't expect a title, but a resource name... I figured using the same example that we have for the complete error would be better.)

The commas were a mistake though.

I've reformatted each entry in the map as code - we could reformat it all as a JSON object if you'd like?

noahdietz · 2024-09-26T21:35:49Z

aip/general/0193.md

-map for the error payload to be backwards compatible, even if the value
-for a particular key is empty. Keys **must** be expressed as lower
-camel-case.
+In other words, once a user has observed a given key for a (reason, domain) pair they should be


Suggested change

In other words, once a user has observed a given key for a (reason, domain) pair they should be

In other words, once a user has observed a given key for a (reason, domain) pair they **must** be

stronger guidance? If not, perhaps just bold the should...or change to something else if you want to just make an assertion e.g. the need to be able to rely...

Rewritten - I tend to think that every "must" and "should" ought to be aimed at the target audience of the AIP - usually the service owner, rather than the user. (Client library AIPs are slightly different.) Each should/must ought to be effectively an instruction - whereas saying "the user must be able to do something" isn't really an instruction to the subject of the verb... see what you think of this rewrite :)

aip/general/0193.md

jskeet

I've made the changes within the existing commit structure (I hope).

It's possible that will lose the comments - but I hope not. (If it does, I'll abandon that approach for the rest of the commits.)

aip/general/0193.md

jskeet · 2024-09-27T09:41:52Z

aip/general/0193.md

+The `message` field is a a developer-facing, human-readable "debug message"
+which **should** be in English. (Localized messages are expressed using
+a `LocalizedMessage` within the `details` field.) Any dynamic aspects of
+the message **must** be included as metadata within the `ErrorInfo` message


Let's talk about this a bit more - I tend to find that it's worth keeping things not in code font when they're not explicitly talking about the field (which this isn't - it's talking about the semantic meaning). I'm happy to be persuaded, and I suspect there will be some areas where I'm inconsistent already... but to my eyes, a page where a huge proportion of the text is in code font ends up being hard to read.

aip/general/0193.md

jskeet · 2024-09-27T10:09:42Z

aip/general/0193.md

+This practice is critical so that machine actors do not need to parse error
+messages to extract information.


Let's leave this open so we can move it and link.

aip/general/0193.md

jskeet · 2024-09-27T10:13:13Z

aip/general/0193.md

+- "zone": "us-east1-a",
+- "vmType": "e2-medium",
+- "attachment": "local-ssd=3,nvidia-t4=2",
+- "zonesWithCapacity": "us-central1-f,us-central1-c"


I'm not sure... it's not really code - they're name/value pairs. (I replaced the example entirely, as I thought it was unrealistic - if we're going to use books, I wouldn't expect a title, but a resource name... I figured using the same example that we have for the complete error would be better.)

The commas were a mistake though.

I've reformatted each entry in the map as code - we could reformat it all as a JSON object if you'd like?

jskeet · 2024-09-27T10:16:20Z

aip/general/0193.md

-map for the error payload to be backwards compatible, even if the value
-for a particular key is empty. Keys **must** be expressed as lower
-camel-case.
+In other words, once a user has observed a given key for a (reason, domain) pair they should be


Rewritten - I tend to think that every "must" and "should" ought to be aimed at the target audience of the AIP - usually the service owner, rather than the user. (Client library AIPs are slightly different.) Each should/must ought to be effectively an instruction - whereas saying "the user must be able to do something" isn't really an instruction to the subject of the verb... see what you think of this rewrite :)

noahdietz

Your responses to my review and the following two commits lgtm

aip/general/0193.md

jskeet

Thanks for the reviews so far - I'll ping again when I've added the commits for the final sections. (We may also want to separate out "everything to do with Status" from "how to decide which error codes to use, but that can be a later PR.)

jskeet · 2024-10-08T14:34:57Z

This is now "finished" (other than adding a dated update line to the bottom) in that I've been through it all. There are quite a few changes which will need further discussion though - and no doubt nits here and there. I would propose that from now on, changes are made as new commits instead of updating the existing ones, if that's okay with everyone.

noahdietz

Overall LGTM

For posterity, we want a few more reviews, specific @shwoodard and @jschneider11

noahdietz · 2024-10-08T19:28:38Z

aip/general/0193.md

+RPCs which have **always** included `ErrorInfo` are in a better position:
+the contract is then more about the stability of `ErrorInfo` for any given
+error: the reason and domain need to be consistent over time, and the
+metadata provided for any given (reason,domain) can only be expanded.


Nit: Is it OK to have multiple colons in the same sentence?

No, it's horrible - sorry about that. I think changing the second colon to a period would be better. Will do that locally, but not create a commit just for that. (I'll wait until we get more feedback to avoid a storm of tiny commits.)

aip/general/0193.md

jskeet · 2024-10-09T10:07:38Z

aip/general/0193.md


 Each type of detail payload **must** be included at most once. For
 example, there **must not** be more than one [`BadRequest`][BadRequest]
 message in the `details`, but there **may** be a `BadRequest` and a
 [`PreconditionFailure`][PreconditionFailure].

-##### ErrorInfo payload
+Structured details with machine-readable identifiers **must** be used so


I'm now considering removing this paragraph entirely. It's already covered later on, and the currnet "if an error message does not have a machine-readable identifier in addition to the message" effectively contradicts the requirement that we always provide an ErrorInfo.

Yeah I'm a little unsure of the value this paragraph is now providing. Agreed that it adds some confusion.

I've tacked just "This provides machine-readable identifiers so that that users can write code against specific aspects of the error" onto the next paragraph (which was otherwise very short anyway). I think that captures the important bit.

aip/general/0193.md

noahdietz · 2024-10-10T19:28:54Z

aip/general/0193.md


 Each type of detail payload **must** be included at most once. For
 example, there **must not** be more than one [`BadRequest`][BadRequest]
 message in the `details`, but there **may** be a `BadRequest` and a
 [`PreconditionFailure`][PreconditionFailure].

-##### ErrorInfo payload
+Structured details with machine-readable identifiers **must** be used so


Yeah I'm a little unsure of the value this paragraph is now providing. Agreed that it adds some confusion.

jschneider11

I've been requested to review, but can't approve this PR.

LGTM, though.

jskeet

I've now left a "potentially last commit" - I had to force push, due to (I suspect) some local modifications, but it's only the last commit that should require re-review.

jskeet · 2024-10-18T14:46:44Z

aip/general/0193.md

+remain the same for any given error, as developers have previously had no
+option but to use this for error handling. For more information, see
+[Changing Error Messages](#changing-error-messages).


Done exactly as stated.

jskeet · 2024-10-18T14:48:40Z

aip/general/0193.md


 Each type of detail payload **must** be included at most once. For
 example, there **must not** be more than one [`BadRequest`][BadRequest]
 message in the `details`, but there **may** be a `BadRequest` and a
 [`PreconditionFailure`][PreconditionFailure].

-##### ErrorInfo payload
+Structured details with machine-readable identifiers **must** be used so


I've tacked just "This provides machine-readable identifiers so that that users can write code against specific aspects of the error" onto the next paragraph (which was otherwise very short anyway). I think that captures the important bit.

jskeet · 2024-10-18T14:56:25Z

aip/general/0193.md

+This practice is critical so that machine actors do not need to parse error
+messages to extract information.


I haven't found a good way of doing this. I'm tempted to suggest we get this PR in but raise an issue to review the new text carefully for rationale-like text - I suspect there's a fair amount of it here. Personally I'm not hugely against it in this particular case, mind you...

noahdietz · 2024-10-18T19:14:16Z

aip/general/0193.md

-library: "Garfield East"
-expectedReturnDate: "2199-05-13"
-```
+The reason should be terse, but meaningful enough for a human reader to


Suggested change

The reason should be terse, but meaningful enough for a human reader to

The reason **should** be terse, but meaningful enough for a human reader to

Nit: Is this a requirement word or should we use a different one (or should we not fuss about use of requirement wrods in a non-requirement context?)

I'm fine for it to be a requirement word. Added a new comment for this and the other example.

noahdietz · 2024-10-18T19:16:12Z

aip/general/0193.md

+This practice is critical so that machine actors do not need to parse error
+messages to extract information.


That's fine with me (punting)

noahdietz · 2024-10-18T19:19:00Z

aip/general/0193.md

- However, the `Status.message` *string* **must** not be changed, as
-  this change is backward-incompatible.
+Textual error messages can be present in both `Status.message` and
+`LocalizedMessage.message` fields. Messages should be succinct but


Suggested change

`LocalizedMessage.message` fields. Messages should be succinct but

`LocalizedMessage.message` fields. Messages **should** be succinct but

ditto re:requirement word or not

This is more of an informative section than anything else - API designers only need to know about it in order to understand the JSON response they might see from curl etc.

(This doesn't include anything about the details "payloads" yet.)

Note: this has removed the term "dynamic variables" in favor of only referring to it as "metadata". It's handy to have a single well-defined term, and given that the field name is `metatadata`, I've kept that.

Note that we don't currently (as far as I'm aware) have a good standardized way of letting users specify a locale. (Can they use Accept-Language, for example? What about in gRPC?) We can tackle that separately. This commit changes the guidance in a couple of important ways: - It says that the locale should be a user-specified one rather than a "service's native language" - It calls out the ability to use LocalizedMessage even when we don't have a user-specified language, when Status.message can't be changed for compatibility reasons This commit *doesn't* address the issue of the target audience of LocalizedMessage; later text claims this is aimed at end-users whereas Status.message is more aimed at developers. I'm not convinced by that at the moment.

Significant changes here (all up for discussion): - Explicitly include Status.message as well as LocalizedMessage.message - Remove the standardized https://cloud.google.com/PRODUCT/docs/ERROR-REASON format requirement - Require the description to be suitable as text for a link - Require the description to be plain text - Require the URL to be absolute and include a scheme - Add authentication requirements

Note: this removes the "message alignment" section which I found confusing. We could restore and edit it should we wish.

Note: this removes the idea that LocalizedMessage is for "end users" as a distinction between that and Status.message being for developers. If we still want that distinction, we should talk about it more and clarify it - in particular, if we're using LocalizedMessage as an alternative for Status.message where the latter can't be changed, they can't really cater for different audiences.

jskeet · 2024-10-21T08:05:43Z

Pushed one extra commit and rebased. Will merge on green.

noahdietz reviewed Sep 26, 2024

View reviewed changes

jskeet force-pushed the update-193 branch from 7cb742a to 635a39b Compare September 27, 2024 10:19

jskeet commented Sep 27, 2024

View reviewed changes

noahdietz reviewed Sep 30, 2024

View reviewed changes

yordis reviewed Sep 30, 2024

View reviewed changes

aip/general/0193.md Show resolved Hide resolved

jskeet commented Oct 4, 2024

View reviewed changes

jskeet marked this pull request as ready for review October 8, 2024 14:33

jskeet requested a review from a team as a code owner October 8, 2024 14:33

jskeet requested a review from itsStrobe October 8, 2024 14:33

jskeet force-pushed the update-193 branch from 623c023 to 0dee1cd Compare October 8, 2024 14:33

jskeet requested review from yordis, noahdietz, jschneider11 and shwoodard and removed request for yordis and jschneider11 October 8, 2024 14:33

noahdietz approved these changes Oct 8, 2024

View reviewed changes

jskeet commented Oct 9, 2024

View reviewed changes

noahdietz approved these changes Oct 10, 2024

View reviewed changes

noahdietz requested review from jschneider11 and removed request for itsStrobe, yordis and jschneider11 October 15, 2024 22:11

jschneider11 reviewed Oct 16, 2024

View reviewed changes

jskeet force-pushed the update-193 branch from 0dee1cd to 7071c0b Compare October 18, 2024 15:01

jskeet commented Oct 18, 2024

View reviewed changes

noahdietz approved these changes Oct 18, 2024

View reviewed changes

jskeet added 9 commits October 21, 2024 09:05

AIP-193: Move JSON representation details to the bottom, and clarify

d9bd41b

This is more of an informative section than anything else - API designers only need to know about it in order to understand the JSON response they might see from curl etc.

AIP-193: First part of restructuring the description of Status fields

bdb00a0

(This doesn't include anything about the details "payloads" yet.)

Rewrite ErrorInfo description

b6e62f9

Note: this has removed the term "dynamic variables" in favor of only referring to it as "metadata". It's handy to have a single well-defined term, and given that the field name is `metatadata`, I've kept that.

AIP-193: Update error messages section (including rationale)

dfcae90

Note: this removes the "message alignment" section which I found confusing. We could restore and edit it should we wish.

AIP-193 review feedback and add a dated changelog entry

fe6c318

AIP-193: Address review comments

068f467

jskeet force-pushed the update-193 branch from 6943468 to 068f467 Compare October 21, 2024 08:05

jskeet merged commit e95cde4 into aip-dev:master Oct 21, 2024
2 checks passed

	the message must be included as metadata within the `ErrorInfo` message
	the `message` must be included as metadata within the `ErrorInfo` message

		This practice is critical so that machine actors do not need to parse error
		messages to extract information.

	In other words, once a user has observed a given key for a (reason, domain) pair they should be
	In other words, once a user has observed a given key for a (reason, domain) pair they must be

	The reason should be terse, but meaningful enough for a human reader to
	The reason should be terse, but meaningful enough for a human reader to

	`LocalizedMessage.message` fields. Messages should be succinct but
	`LocalizedMessage.message` fields. Messages should be succinct but

Restructure AIP-193 #1426

Restructure AIP-193 #1426

Conversation

jskeet commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahdietz left a comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

jskeet commented Oct 8, 2024

noahdietz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschneider11 left a comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Oct 21, 2024