10 ms·

Well I commend the Nginx team for trying something new and launching this, even though I'm not sure what I would use it for personally. Slightly tangental, but

Well I commend the Nginx team for trying something new and launching this, even though I'm not sure what I would use it for personally. Slightly tangental, but it always irks me when I see these kinds of responses in JSON: { "success": "Reconfiguration done." } Really this should be something like "result": "success". Using "success" as a key name tells me nothing about the data it's representing.

dan-robertson 3y ago
I think this is trying to be a discriminated union type[1] in json. One way to do it is to have a ‘tag’ field that says “ok” or “err” or whatever and then other fields for the rest of the data (or an array with the tag in the first slot). Another is to have one field whose name is the tag and whose value is everything else (which is what happened here). [1] eg one where the possible values are either Err(<some error>) or Ok(<some result>), and the data inside could be more complex types instead of just strings
9dev 3y ago
Heh. My pet peeve are useless success fields. A 200 response tells you it was successful already. We can talk about a „message“ field or something if you intend to display it to a user, but even that should be implicit by being a response to a specific request. If I send a POST /configuration, is there really something new to be gained from that message that the client cannot figure out on its own? Wouldn’t, perhaps, a 201 No Content suffice?
- eterps 3y ago
  Eric S. Raymond, in his guide "The Art of Unix Programming", mentions this principle as Rule of Silence: "When a program has nothing surprising to say, it should say nothing." His statement probably related to command line applications, but it makes sense for a lot of cases.
  - pydry 3y ago
    It sounds smart but what counts as surprising is entirely context dependent and most programs won't be aware of your context. E.g. a command line app where you put a subtly wrong switch in that does exactly what it thought you wanted and prints nothing while outputting a 0 exit code is dangerous.
    - loloquwowndueo 3y ago
      What, like rm -rf ?
    - couchand 3y ago
      It's about context, of course, but it's not really the program's responsibility to know the user's context. It's the user's responsibility to understand their own context. A tool should have sufficient interlocks to ensure safety when not engaged, but no more.
    - 9dev 3y ago
      I follow your point, however it actually supports mine: An HTTP response isn’t intended to be the output of a command line app. A command line app should take the response from the lower-level HTTP communication and articulate whatever appropriate message to the user, depending on the verbosity level for example. Talking in OSI terms - this is a presentation layer problem, not a link layer one.
  - laserDinosaur 3y ago
    "When a program has nothing surprising to say, it should say nothing." Which sounds good in theory, until you run into issues like this: https://www.youtube.com/watch?v=tLdRBsuvVKc https://www.youtube.com/watch?v=tLdRBsuvVKc TLDW: Most of the Gitlabs outage where they deleted the primary prod and backup prod database could have been solved if they just waited for the command line op to complete - but because of a lack of feedback they thought it had frozen, leading to several other plans which made the situation worse.
_cenw 3y ago
Unit has been around for half a decade. It feels like an evolution to something like Phusion Passenger, but it's not quite cloud-native. A lot of the documentation is tailored to installing directly on a server and some essentials (i.e. prometheus metrics) are missing. I briefly evaluated it for bringing a PHP team into our Kubernetes cluster, but then ended up writing a bit of Go code to proxy into a real nginx+fcgi while adding a syslog sink (so PHP could log to stdout/stderr) and our standard prometheus metrics on the Go http server.
- fideloper 3y ago
  wait, did that continue to use php-fpm? if not I want details plz! :D (especially around the logging sync, unless it’s just php-fpm configured to collect child process output)
  - _cenw 3y ago
    Yes, it did, not too much magic going on, the Go proxy also was a replacement for supervisord so it started nginx and fpm, reaped zombies and pulled all the important logs into stdout/stderr. The most remarkable thing was how this revitalized a team that wrote boring PHP software limited to deploying via FTP on PHP 5. They ended up really getting into stripping out every piece of PHP they didn't use because the image built PHP from scratch, eventually took over maintenance of the go piece and ported a bunch of their apps to Symphony. I was in a platform engineering team and they were one of the few teams to really torture test every feature we ever shipped to the point they'd report edge cases or a bug to us every other week or so. As for the logging question, we configured Symphony to log to syslog, which was provided by the Go daemon via unix socket.
    - fideloper 3y ago
      awesome, thank you for the details!
- aforwardslash 3y ago
  depending on the desired metrics, the Unit /status endpoint may provide enough info, and transformation of json to prometheus metrics is somewhat easy with existing tooling
chrismorgan 3y ago
In this particular example, it’s actually a whole lot worse than that, by spec at least. Excerpt from their OpenAPI schema <https://github.com/nginx/unit/blob/7dd5ad93a4c147b086a8d82ecb403f96981bb4ee/docs/unit-openapi.yaml https://github.com/nginx/unit/blob/7dd5ad93a4c147b086a8d82ec...>: jsonSuccessMessage: type: object description: "JSON message on success." additionalProperties: type: string jsonErrorMessage: type: object description: "JSON message on error." additionalProperties: type: string Yes, this is as bad as it looks: “success” isn’t even part of the schema. It’s in the examples, but not the actual schema definition. The way success or failure is actually indicated is by HTTP status codes. 200 is success, 400/404/500 is error. So, they seem to set very bad precedent in at least one of their response and their schema. But I was expecting it to be something like {"success": string} | {"error": string}, which is frankly a perfectly reasonable way of doing things: an untagged, but still unambiguous, union. It can make for quite pleasant code, too: `if response.success` and such. You’re looking for a tagged union, something like {"result": "success" | "error", "message": string} (or alternatively like {"result": "success", "message": string} | {"result": "error", "code": string, …}), which is also a perfectly reasonable way of doing things. In some ways it can be viewed as more principled, as it more formally allows you to check the tag. Really, the two approaches are much of a muchness. They each have their strengths and their weaknesses. But if general status is being done at the HTTP layer, I’d prefer {"message": "Reconfiguration done."}, or… well, actually, just a 204 No Content response, and no JSON or body at all. (On the terms untagged and tagged unions as I’m using them: they’re necessarily a bit different from the concepts as exposed in languages like C, since you’re dealing with a model built on objects rather than bytes, but they’re still reasonable descriptions of them. In Serde’s classifications, they’d be the untagged and internally tagged enum representations <https://serde.rs/enum-representations.html https://serde.rs/enum-representations.html>.)
- Nullabillity 3y ago
  By Serde's classification, `success: {}` is a textbook externally tagged representation.
  - mindwok 3y ago
tanepiper 3y ago
Regarding use case. We have a bunch of frontend apps that are integrations to one of our external platforms - they get hosted in an iframe on there. To simplify the builds we have a expressjs app that is a simple static server and proxy for API requests. I can see something like this easily replace that - all we need is static files and a /API* endpoint to proxy - avoiding cors issues.
tecleandor 3y ago
That's like when I see APIs that give you responses with a 200 code, but then in the message there's a backend error.
- FridgeSeal 3y ago
  Looks pointedly at graphQL projects.
- smashed 3y ago
  Some people treat http as a pure transport/network layer and the actual body as the app-level layer. In such an approach, any http-level error would be a network, server maintenance or other unknown/fatal error type of things. App errors are encoded in the response body in a app-defined way. It's not a completely bogus way of handling things, as long as it is perfectly consistent throughout the project and properly documented, which is rarely the case.
- wruza 3y ago
  The opposite of that is people who try to loosely project the variety of error modes their app has to http status codes. You then print out a table of http code <-> actual meaning and try to outline ranges and messages you want to handle differently. On top of that, their backend may be down and you have to deal with bare reverse proxy statuses as well, which adds another dimension to that mess.
revskill 3y ago
Should it be { status: 200, body: { "message": "Reconfiguration done." } } ?
afavour 3y ago
If I were designing it I’d probably have a result field too, or even a success boolean. But I don’t hate what they’re doing here: the presence of the key is the Boolean value, the value is the description. A two for one.
- wruza 3y ago
  An economy not worth investing in in my book. It’s always more clear to separate success and result. Sometimes, you’ll have no result, so there will be some parasitic (or truthy) value like null or true. And when you check success-named result deeper/further in code, it looks like you’re indexing into a boolean. Trying to make it clear on-site creates miniprotocols not worth remembering. Worst case scenario is {success:x, error:y}, x and y being 4 combinations of null and non-null, where you aren’t even sure what happened and may swallow a false positive with an optimistic check. Also, since undefined is not in json standard, the existence becomes ephemeral in languages where that’s distinct from just being undefined. You may think that returning undefined from a wrapped worker is okay, but it results in “error encountered: undefined” down the line due to {} payload. It’s one of the things that make programming harder for no good reason.
giancarlostoro 3y ago
I rather have: { "responseMessage": "Reconfiguration done.", "responseCode": 133, "isSuccessful": true } This way you can map to a custom message on your front-end based on the responseCode whether its successful or not, you can fallback to the response message if no mapping is found, and you can easily check if the transaction was successful or not.
maxloh 3y ago
Why including that in the response body? 200 status code states everything.
deleted 3y ago
[deleted]

dan-robertson 3y ago

I think this is trying to be a discriminated union type[1] in json. One way to do it is to have a ‘tag’ field that says “ok” or “err” or whatever and then other fields for the rest of the data (or an array with the tag in the first slot). Another is to have one field whose name is the tag and whose value is everything else (which is what happened here). [1] eg one where the possible values are either Err(<some error>) or Ok(<some result>), and the data inside could be more complex types instead of just strings

9dev 3y ago

Heh. My pet peeve are useless success fields. A 200 response tells you it was successful already. We can talk about a „message“ field or something if you intend to display it to a user, but even that should be implicit by being a response to a specific request. If I send a POST /configuration, is there really something new to be gained from that message that the client cannot figure out on its own? Wouldn’t, perhaps, a 201 No Content suffice?

eterps 3y ago
Eric S. Raymond, in his guide "The Art of Unix Programming", mentions this principle as Rule of Silence: "When a program has nothing surprising to say, it should say nothing." His statement probably related to command line applications, but it makes sense for a lot of cases.
- pydry 3y ago
  It sounds smart but what counts as surprising is entirely context dependent and most programs won't be aware of your context. E.g. a command line app where you put a subtly wrong switch in that does exactly what it thought you wanted and prints nothing while outputting a 0 exit code is dangerous.
  - loloquwowndueo 3y ago
    What, like rm -rf ?
  - couchand 3y ago
    It's about context, of course, but it's not really the program's responsibility to know the user's context. It's the user's responsibility to understand their own context. A tool should have sufficient interlocks to ensure safety when not engaged, but no more.
  - 9dev 3y ago
    I follow your point, however it actually supports mine: An HTTP response isn’t intended to be the output of a command line app. A command line app should take the response from the lower-level HTTP communication and articulate whatever appropriate message to the user, depending on the verbosity level for example. Talking in OSI terms - this is a presentation layer problem, not a link layer one.
- laserDinosaur 3y ago
  "When a program has nothing surprising to say, it should say nothing." Which sounds good in theory, until you run into issues like this: https://www.youtube.com/watch?v=tLdRBsuvVKc https://www.youtube.com/watch?v=tLdRBsuvVKc TLDW: Most of the Gitlabs outage where they deleted the primary prod and backup prod database could have been solved if they just waited for the command line op to complete - but because of a lack of feedback they thought it had frozen, leading to several other plans which made the situation worse.

_cenw 3y ago

Unit has been around for half a decade. It feels like an evolution to something like Phusion Passenger, but it's not quite cloud-native. A lot of the documentation is tailored to installing directly on a server and some essentials (i.e. prometheus metrics) are missing. I briefly evaluated it for bringing a PHP team into our Kubernetes cluster, but then ended up writing a bit of Go code to proxy into a real nginx+fcgi while adding a syslog sink (so PHP could log to stdout/stderr) and our standard prometheus metrics on the Go http server.

fideloper 3y ago
wait, did that continue to use php-fpm? if not I want details plz! :D (especially around the logging sync, unless it’s just php-fpm configured to collect child process output)
- _cenw 3y ago
  Yes, it did, not too much magic going on, the Go proxy also was a replacement for supervisord so it started nginx and fpm, reaped zombies and pulled all the important logs into stdout/stderr. The most remarkable thing was how this revitalized a team that wrote boring PHP software limited to deploying via FTP on PHP 5. They ended up really getting into stripping out every piece of PHP they didn't use because the image built PHP from scratch, eventually took over maintenance of the go piece and ported a bunch of their apps to Symphony. I was in a platform engineering team and they were one of the few teams to really torture test every feature we ever shipped to the point they'd report edge cases or a bug to us every other week or so. As for the logging question, we configured Symphony to log to syslog, which was provided by the Go daemon via unix socket.
  - fideloper 3y ago
    awesome, thank you for the details!
aforwardslash 3y ago
depending on the desired metrics, the Unit /status endpoint may provide enough info, and transformation of json to prometheus metrics is somewhat easy with existing tooling

chrismorgan 3y ago

In this particular example, it’s actually a whole lot worse than that, by spec at least. Excerpt from their OpenAPI schema <https://github.com/nginx/unit/blob/7dd5ad93a4c147b086a8d82ecb403f96981bb4ee/docs/unit-openapi.yaml https://github.com/nginx/unit/blob/7dd5ad93a4c147b086a8d82ec...>: jsonSuccessMessage: type: object description: "JSON message on success." additionalProperties: type: string jsonErrorMessage: type: object description: "JSON message on error." additionalProperties: type: string Yes, this is as bad as it looks: “success” isn’t even part of the schema. It’s in the examples, but not the actual schema definition. The way success or failure is actually indicated is by HTTP status codes. 200 is success, 400/404/500 is error. So, they seem to set very bad precedent in at least one of their response and their schema. But I was expecting it to be something like {"success": string} | {"error": string}, which is frankly a perfectly reasonable way of doing things: an untagged, but still unambiguous, union. It can make for quite pleasant code, too: `if response.success` and such. You’re looking for a tagged union, something like {"result": "success" | "error", "message": string} (or alternatively like {"result": "success", "message": string} | {"result": "error", "code": string, …}), which is also a perfectly reasonable way of doing things. In some ways it can be viewed as more principled, as it more formally allows you to check the tag. Really, the two approaches are much of a muchness. They each have their strengths and their weaknesses. But if general status is being done at the HTTP layer, I’d prefer {"message": "Reconfiguration done."}, or… well, actually, just a 204 No Content response, and no JSON or body at all. (On the terms untagged and tagged unions as I’m using them: they’re necessarily a bit different from the concepts as exposed in languages like C, since you’re dealing with a model built on objects rather than bytes, but they’re still reasonable descriptions of them. In Serde’s classifications, they’d be the untagged and internally tagged enum representations <https://serde.rs/enum-representations.html https://serde.rs/enum-representations.html>.)

Nullabillity 3y ago
By Serde's classification, `success: {}` is a textbook externally tagged representation.
- mindwok 3y ago

tanepiper 3y ago

Regarding use case. We have a bunch of frontend apps that are integrations to one of our external platforms - they get hosted in an iframe on there. To simplify the builds we have a expressjs app that is a simple static server and proxy for API requests. I can see something like this easily replace that - all we need is static files and a /API* endpoint to proxy - avoiding cors issues.

tecleandor 3y ago

That's like when I see APIs that give you responses with a 200 code, but then in the message there's a backend error.

FridgeSeal 3y ago
Looks pointedly at graphQL projects.
smashed 3y ago
Some people treat http as a pure transport/network layer and the actual body as the app-level layer. In such an approach, any http-level error would be a network, server maintenance or other unknown/fatal error type of things. App errors are encoded in the response body in a app-defined way. It's not a completely bogus way of handling things, as long as it is perfectly consistent throughout the project and properly documented, which is rarely the case.
wruza 3y ago
The opposite of that is people who try to loosely project the variety of error modes their app has to http status codes. You then print out a table of http code <-> actual meaning and try to outline ranges and messages you want to handle differently. On top of that, their backend may be down and you have to deal with bare reverse proxy statuses as well, which adds another dimension to that mess.

revskill 3y ago

Should it be { status: 200, body: { "message": "Reconfiguration done." } } ?

afavour 3y ago

If I were designing it I’d probably have a result field too, or even a success boolean. But I don’t hate what they’re doing here: the presence of the key is the Boolean value, the value is the description. A two for one.

wruza 3y ago
An economy not worth investing in in my book. It’s always more clear to separate success and result. Sometimes, you’ll have no result, so there will be some parasitic (or truthy) value like null or true. And when you check success-named result deeper/further in code, it looks like you’re indexing into a boolean. Trying to make it clear on-site creates miniprotocols not worth remembering. Worst case scenario is {success:x, error:y}, x and y being 4 combinations of null and non-null, where you aren’t even sure what happened and may swallow a false positive with an optimistic check. Also, since undefined is not in json standard, the existence becomes ephemeral in languages where that’s distinct from just being undefined. You may think that returning undefined from a wrapped worker is okay, but it results in “error encountered: undefined” down the line due to {} payload. It’s one of the things that make programming harder for no good reason.

giancarlostoro 3y ago

I rather have: { "responseMessage": "Reconfiguration done.", "responseCode": 133, "isSuccessful": true } This way you can map to a custom message on your front-end based on the responseCode whether its successful or not, you can fallback to the response message if no mapping is found, and you can easily check if the transaction was successful or not.

maxloh 3y ago

Why including that in the response body? 200 status code states everything.

deleted 3y ago

[deleted]