Ship call logs to the user as text/plain instead of JSON #615

denismakogon · 2017-12-20T22:59:06Z

Fn API changed.
Swagger updated.
Certain API tests were disabled to make CI happy.

skinowski · 2017-12-21T04:16:30Z

api/server/call_logs.go

-
-	c.JSON(http.StatusOK, callLogResponse{"Successfully loaded log", &callObj})
+	c.Header("Content-Type", "text/plain")
+	c.JSON(http.StatusOK, b.String())


From docs, it sounds like c.JSON() is going to set the content-type to json again?

https://godoc.org/github.com/gin-gonic/gin#Context.JSON

maybe c.String() or c.Data() would work better?

Yeah, but it’s funny, line above that set content type to text worked, so the response content type at client side was text/plain, but not json

rdallman

I'm concerned about breaking this API. I don't think there's any reason we can't return both a JSON or a plaintext from this endpoint, based on Accept headers. I'd also like to start storing all our logs in gzip form and returning a gzip string from here which will be similar in nature (Accept-Encoding). I have a little patch laying around playing with the gzip stuff, I can dust it off and try to finish it or we can just do the Accept header stuff here and add gzip later, it's not too complex so you can take over if you'd like. But at this point I don't think we should be breaking the API since a bunch of people have CLI laying around now, esp when we can easily support the old API and just upgrade clients around the updated API.

let me know re: gzip

oops

rdallman

i hate they default to approve -.-

rdallman · 2017-12-21T17:57:22Z

api/datastore/internal/datastoreutil/metrics.go

+		return err
+	}
+
+	return m.ds.InsertLog(ctx, appName, callID, newCallLog)


let's punt on this (didn't get a reply from you). we should discuss. we want to thread gzip through the whole system really, it's not this straightforward. then we aren't gathering un-compressed bytes (in memory/files) and then compressing them just on upload. let's just fix plain text api here

metric ds is kind of the wrong place to put this, too.

anyway i ran into complications trying that (hence my abandonment). but now i know what was wrong (there was another since fixed bug in logs)

I'm not sure i understand your first comment.

So, if that is not metrics ds, so gzip compressing should be just in every logstore implementation, which doesn't seem reasonable.

metric ds is not the only option. i don't think we need any implementation. i can dig up my patch and try to explain more concretely, am happy to push forward with the plaintext api meanwhile (based on Accept headers)

there's one problem - swagger, not sure that they support multiple kinds of responses in different formation: same response code, but the body is different.

i'd like to deal with gzip thing in first place, only then plaintext api thing.

we can change swagger to plaintext, that's fine, and new clients will use it, but we need backwards compat for old clients using json still. the gzip isn't pressing, really, and there's code that has to be written for compatibility so it's not perfectly straightforward

Done, API remains compatible with older clients. So, i'd like to know your thoughts and how you see gzip'ing function's logs.

As far as i understand there's nothing much code needed to do actual compressing, right? But the bigger question to me is where it happens in order to be the same for all stores (sql and minion at this moment, obviously it should happen just right before going to actual implementations).

rdallman · 2017-12-21T23:07:21Z

api/server/call_logs.go

-		AppName: appName,
-		Log:     b.String(),
+	switch c.Request.Header.Get("Accept") {
+	case "application/json":


this only gets the first value from Accept, which may be a list, and we likely want to prefer plaintext

I do aware that Accept can be a list, but fn_go sends exactly one item (which is application/json). So the best option i see here is to loop over MIME types.

ok, for the old endpoint that makes sense. do the updated swagger clients request text/plain properly for this endpoint?

denismakogon · 2017-12-22T19:55:25Z

Okay, the swagger doc is updated. Will push new client(s) once swagger doc will be at the master.

rdallman · 2017-12-23T16:30:08Z

api/server/call_logs.go

+	for _, mimeType := range mimeTypes {
+		switch mimeType {
+		case "text/plain":
+			c.String(http.StatusOK, b.String())


one of the nice benefits of having a plaintext api here is that we don't need to buffer and encode the log. we should only read the log into a buffer for the deprecated json path and for the new plaintext api, we can just copy the log directly to the response connection. for minio/s3 backend this will read directly from the s3 connection and copy it to the client response which means we aren't buffering it at all internally, which is pretty slick.

rdallman · 2017-12-23T16:32:06Z

aside from fixing the buffer usage, would be great to know 2 things before approving:

this code works with the old swagger clients, for sure (pick one, cli even)
the plaintext path is executed with the newly generated swagger clients. if not, can we fix our swagger doc to make this possible?

in addition to that, an api test that covers both the Accept: application/json and Accept: text/plain and makes sure that the output is what we expect would be great (also wrt priorities if both are given)

denismakogon · 2017-12-23T19:11:17Z

Answering your questions:

This code works with old swagger clients (API tests are passing and Logs API tests are there).
API works with the new client (patch for Ship logs as text cli#128).

Regarding API test, okay, that's possible.

rdallman · 2017-12-26T17:07:12Z

thanks. please fix #615 (comment) -- for plaintext, the code should read:

io.Copy(c.ResponseWriter, logReader)

with no buffer in between

edit: you may also need to set the content type explicitly since go detection may detect something else from the log, and we do not want to use the gin String method, as we don't want to turn this into a string it's a wasteful [large] allocation.

treeder · 2017-12-27T17:05:25Z

I see that a lot of code was rolled back in this PR, but why did/do we want to change the default response to plain/text instead of json like it was? I get the reason that someone might want this, but I would find it strange that a particular endpoint returns plain text by default while the rest return JSON.

denismakogon · 2017-12-27T17:51:25Z

So, there's at least one reason why we should not ship logs as JSON. It forces us to do buffering of the whole log object which can actually be pretty huge. The second reason is JSON that we ship at this moment is mostly useless because users would need just log entity, why do they need exactly the same call ID as they passed in the request (just remember we dropped app name from router response object). Without call ID current JSON response is nothing but a JSON with 1 item only. So, tell me what's the point of having a JSON response that forces us to buffer the log at the server and do JSON parsing at the client side, why not just ship log as text?

Regarding amendments, at 1st iteration, i decided to implement gzip thing for making logs smaller, but @rdallman had concerns about the implementation, so we decided to stick with 1st part of the original issue - ship log as text, and as the follow-up - use gzip to shrink logs.

treeder · 2017-12-28T15:03:12Z

Well, consistency for one. Every endpoint returns JSON by default except one.

And two, there's a high probability we could add more fields.

I like leaving it up to the user to pass in text/plain if they want to change to another format, but default should probably remain JSON. If it's too big of a log (which would take a lot and we probably shouldn't be storing it anyways), we could return an error and tell them to pass in text/plain to retrieve it.

rdallman · 2018-01-02T15:29:09Z

it seems fine to default to json, I guess users may expect this if they don't provide a content type. even with jq magic the logs are pretty hard to parse out (via eyeball) of json since the newline chars get escaped fwiw. at least in our clients we generate and in the cli we can send in Accept: text/plain and get back a plain text log, which should cover most usage (so we get the benefits). wrt fields, at a minimum we can maintain some endpoint that just returns a plaintext log, whether it's the current one or not -- we have .../call/:id for additional call fields, for most stuff probably.

rdallman · 2018-01-02T15:35:05Z

api/server/call_logs.go

 	mimeTypes, _ := c.Request.Header["Accept"]

 	for _, mimeType := range mimeTypes {
 		switch mimeType {
 		case "text/plain":
-			c.String(http.StatusOK, b.String())
+			io.Copy(c.Writer, logReader)
+			c.Status(http.StatusOK)


the status code is already written at this point, can either remove this or move it before io.Copy, either works.

rdallman · 2018-01-02T16:03:23Z

api/server/call_logs.go

-		Log:     b.String(),
+	mimeTypes, _ := c.Request.Header["Accept"]
+
+	for _, mimeType := range mimeTypes {


this isn't the behavior we want. an example:

Accept: application/xml, text/plain will return a json blob. the user clearly can handle text/plain but it's not explicit whether they can handle json, we should return text/plain since it was specified and json was not.

One suggestion might be switch on exact Accept headers in the loop, since we do want to treat them as a prioritized list, and only default to json if we don't find any of the types we expect. the HTML spec pretty clearly defines the behavior we should expose here: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.1 and we also need to handle q values (which won't be switched on properly in this code); for q values, I don't think we need to worry about quality reduction [for now/ever], just to parse them properly. in the spec, if any Accept header is provided but we don't find either application/json or text/plain we're supposed to return 406. If no Accept is provided (which implies */*) it seems like we've agreed to return application/json. I like the idea of sticking to the spec (adding 406 possibility).

edit: some clarification of spec finer details.

rdallman · 2018-01-02T16:06:13Z

api/server/call_logs.go

 	"net/http"

 	"github.com/fnproject/fn/api"
 	"github.com/fnproject/fn/api/models"
 	"github.com/gin-gonic/gin"
 )

+// note: for backward compatibility, will go away later


i don't think this is really true, we should probably support json perpetually if somebody asks for it

rdallman · 2018-01-02T19:27:44Z

api/server/call_logs.go

+		}
+	}
+	// if we've reached this point it means that Fn didn't recognize Accepted content type
+	c.Status(http.StatusNotAcceptable)


if no accept headers are given, we also want to return a json. this is only if they provide accept headers, none of which we can offer.

p.s. extract the function for the json logic, please

please add a body to this response, as well

previous version was producing the following "couldn't find a swagger spec"

rdallman · 2018-01-02T21:12:50Z

api/server/call_logs.go

+		}
+	}
+	// if we've reached this point it means that Fn didn't recognize Accepted content type
+	WriteError(c, c.Writer, http.StatusNotAcceptable,


every other call in the front end uses handleErrorResponse, with an error that implements models.APIError. please do the same here, so that we might log this in handleErrorResponse, add a metric for it, whatever else we may want to do in there so that we have telemetry around this and uniformity in the front end error handling.

rdallman

thanks!

denismakogon requested review from rdallman and skinowski December 20, 2017 22:59

skinowski reviewed Dec 21, 2017

View reviewed changes

rdallman previously approved these changes Dec 21, 2017

View reviewed changes

rdallman suggested changes Dec 21, 2017

View reviewed changes

rdallman reviewed Dec 21, 2017

View reviewed changes

denismakogon force-pushed the ship-log-as-text branch from 8ce792d to 9893c56 Compare December 21, 2017 22:40

rdallman reviewed Dec 21, 2017

View reviewed changes

rdallman reviewed Dec 23, 2017

View reviewed changes

denismakogon mentioned this pull request Dec 23, 2017

Ship logs as text fnproject/cli#128

Merged

denismakogon force-pushed the ship-log-as-text branch 2 times, most recently from 961b33a to 54a2b44 Compare December 23, 2017 20:02

denismakogon force-pushed the ship-log-as-text branch from 54a2b44 to 6fed1d5 Compare December 27, 2017 17:44

rdallman reviewed Jan 2, 2018

View reviewed changes

denismakogon force-pushed the ship-log-as-text branch from 6fed1d5 to b499026 Compare January 2, 2018 18:50

rdallman reviewed Jan 2, 2018

View reviewed changes

denismakogon added 3 commits January 2, 2018 22:32

Ship call logs to the user as text/plain instead of JSON

5f30e0f

Fixing swagger doc

69dc53b

c.String instead of c.JSON

57c304b

denismakogon added 10 commits January 2, 2018 22:32

Make Logs API backward compatible

13fa2cc

Loop over accepted MIME types

a748619

Bump swagger API version

da19a0c

Fix client build script

6188b26

previous version was producing the following "couldn't find a swagger spec"

Logs API regression test

fcbc008

Write response body without buffering

55412cd

Switch JSON and text/plain cases

e92753e

Handle Accepted content types properly

1d2d6d6

More solid response content type handling

fca355d

Write HTTP 406 with corresponding error body

da7e2f8

denismakogon force-pushed the ship-log-as-text branch from 8818917 to da7e2f8 Compare January 2, 2018 20:34

Remove unused import

7475096

rdallman reviewed Jan 2, 2018

View reviewed changes

Use handleErrorResponse

2dd260d

rdallman approved these changes Jan 2, 2018

View reviewed changes

rdallman merged commit 60d2ca2 into master Jan 2, 2018

rdallman deleted the ship-log-as-text branch January 2, 2018 22:01

rdallman mentioned this pull request Jan 2, 2018

log api should offer plaintext #279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ship call logs to the user as text/plain instead of JSON #615

Ship call logs to the user as text/plain instead of JSON #615

denismakogon commented Dec 20, 2017

skinowski Dec 21, 2017

denismakogon Dec 21, 2017

rdallman left a comment

rdallman left a comment

rdallman Dec 21, 2017

rdallman Dec 21, 2017

rdallman Dec 21, 2017

denismakogon Dec 21, 2017

rdallman Dec 21, 2017

denismakogon Dec 21, 2017

denismakogon Dec 21, 2017

rdallman Dec 21, 2017

denismakogon Dec 21, 2017

rdallman Dec 21, 2017

denismakogon Dec 22, 2017

rdallman Dec 22, 2017

denismakogon commented Dec 22, 2017

rdallman Dec 23, 2017

rdallman commented Dec 23, 2017 •

edited

denismakogon commented Dec 23, 2017

rdallman commented Dec 26, 2017 •

edited

treeder commented Dec 27, 2017

denismakogon commented Dec 27, 2017

treeder commented Dec 28, 2017

rdallman commented Jan 2, 2018

rdallman Jan 2, 2018

rdallman Jan 2, 2018 •

edited

rdallman Jan 2, 2018

rdallman Jan 2, 2018

rdallman Jan 2, 2018

rdallman Jan 2, 2018

rdallman left a comment

Ship call logs to the user as text/plain instead of JSON #615

Ship call logs to the user as text/plain instead of JSON #615

Conversation

denismakogon commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdallman left a comment

Choose a reason for hiding this comment

rdallman 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

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

denismakogon commented Dec 22, 2017

Choose a reason for hiding this comment

rdallman commented Dec 23, 2017 • edited

denismakogon commented Dec 23, 2017

rdallman commented Dec 26, 2017 • edited

treeder commented Dec 27, 2017

denismakogon commented Dec 27, 2017

treeder commented Dec 28, 2017

rdallman commented Jan 2, 2018

Choose a reason for hiding this comment

rdallman Jan 2, 2018 • edited

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

rdallman left a comment

Choose a reason for hiding this comment

rdallman commented Dec 23, 2017 •

edited

rdallman commented Dec 26, 2017 •

edited

rdallman Jan 2, 2018 •

edited