Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intake API responses: Body and content type investigation/thoughts #6

Closed
watson opened this issue Aug 4, 2017 · 3 comments
Closed
Labels

Comments

@watson
Copy link
Contributor

watson commented Aug 4, 2017

I just did a little test to see what type of data the server responds with in case of an issue. I tried to trigger all types of errors below 5xx (didn't have a proper way of generating a 5xx response).

All the responses I saw used the correct HTTP status code. That's good. But all responses used text/plain and not application/json as I would have expected.

For an HTTP API that's primarily consumed programmatically, we should prioritise responding with a format that's easily parsable by a computer. Below is how our respones look today.

Invalid URL (result: 404)

% curl localhost:8080/invalid
404 page not found

Invalid HTTP method (result: 405)

% curl localhost:8080/v1/errors
Only post requests are supported

Invalid Content-Type (result: 400)

% curl -X POST -H 'Content-Type: text/plain' localhost:8080/v1/errors
Decoding error: invalid content type: text/plain

Data validation error (result: 400)

% curl -X POST -H 'Content-Type: application/json' -d '{"app":{"name":"foo","agent":{"name":"foo","version":"1"}},"errors":[{}]}' localhost:8080/v1/errors
Data Validation error: Problem validating JSON document against schema: I[#] S[#] doesn't validate with "error#"
  I[#/errors/0] S[#/properties/errors/items/anyOf] anyOf failed
    I[#/errors/0] S[#/properties/errors/items/anyOf/0/required] missing properties: "exception"
    I[#/errors/0] S[#/properties/errors/items/anyOf/1/required] missing properties: "log"%

Notice how the 400 respones repeats the message twice but uses a slightly different wording, e.g. both "Data Validation error" and "Problem validating JSON document against schema". I suggest we just use the former.

Personally I always respond with an empty body in for instance 404 and 405 responses - as no extra info is needed. But since curl by default doesn't display the status code, there is a certain merit in displaying some text to a human for debugging purposes. So we can choose to continue responding with a body if it helps - but I'm also fine with just leaving the body empty in that case. We could also let this depend on the Accept header (curl will set Accept: */* by default, but our agents could set Accept: application/json)

The JSON Schema validation errors seem to contain line breaks and other special formatting that I expect comes directly from the JSON validator. It would be nice if this could be cleaned up in some way.

We had previously talked about always responding with JSON, e.g:

{"message": "foo bar baz"}

What do you think about implementing this? Would love to hear pros/cons 😃

@watson watson added the discuss label Aug 4, 2017
@roncohen
Copy link
Contributor

roncohen commented Aug 7, 2017

I agree that we should include more descriptive error messages. This will be particularly useful for signaling that the apm-server internal queue is full or that it can't contact the upstream Elasticsearch (or kafka) etc. These will be 5xx responses.

In the apm-server, we'd include the error JSON payload if the agent sent an Accept header that allows for it (*/* or application/json etc.) and just return a plain text error if not.

I like error instead of message:

{"error": "foo bar baz"}

When looking at the payload you might not see that the response code is not a 2xx (and as you wrote, curl by default doesn't display the status code), so the error key instead of the message key clearly indicates that there was a failure. I don't feel strongly about it though.

@roncohen
Copy link
Contributor

@watson can we agree on the above?

@watson
Copy link
Contributor Author

watson commented Aug 11, 2017

@roncohen sounds good to me 👍 And good point with using error instead of message.

What do you want to do with 404 and 405 responses? Should they have a body at all? If so, I imagine that body should follow the Accept header as well.

ruflin pushed a commit that referenced this issue Aug 14, 2017
This makes a simple check on the Accept header to determine if the client will understand a JSON response and then sends back:

{"error": "Cannot compare apples to oranges"}
It will fall back to plain text messages if the Accept header is not present or does not indicate JSON compatibility.

Closes #6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants