Should tracing headers be included in 304 Not Modified responses?
mitar opened this issue · 14 comments
I could not find any information about this. But my understanding is that any header included back to the client in the 304 Not Modified response gets merged with headers of the cached resource. This means that if I return tracing headers based on the new request they will get merged back with the original request's response, overriding those tracing headers there. Is there any guideline if tracing headers should be issued or not in 304 Not Modified response?
I see the options:
- If they are issued, then they override the original tracing headers, so now the cached response looks like it was made by a new request. Which might be confusing because the new request might have exited early (because it could return 304 Not Modified) so the content of the original response and tracing headers of the new response might be inconsistent.
- If tracing headers are not issued, then all information helped with tracing debugging 304 Not Modified response might be lost.
then they override the original tracing headers
why do you think so? I don't think we say anywhere that response headers override any request headers.
Not in this spec, but in RFC 2068, see section "Combining Headers":
The end-to-end headers stored in the cache entry
are used for the constructed response, except that any end-to-end
headers provided in the 304 response MUST replace the corresponding
headers from the cache entry. Unless the cache decides to remove the
cache entry, it MUST also replace the end-to-end headers stored with
the cache entry with corresponding headers received in the incoming
response.
That section only refers to "end-to-end headers" which are those explicitly defined in that RFC. That definition does not extend to all possible headers.
Not really, see this section:
The following HTTP/1.1 headers are hop-by-hop headers:
o Connection
o Keep-Alive
o Public
o Proxy-Authenticate
o Transfer-Encoding
o UpgradeAll other headers defined by HTTP/1.1 are end-to-end headers.
Unless you claim that other headers not specified in that concrete RFC are not end-to-end headers? I do not understand it that way. I understand it to mean any header used in the HTTP/1.1 protocol which is not on that list.
Just to understand this better: Are you talking about traceresponse headers here? Please note that traceresponse headers are not yet part of the standard, they are in the "Level 3" of the spec which is in draft right now, and we are working on standardizing them.
Yes, sorry. This is about response headers.
I think this spec should not mandate one or the other. This decision should be made by the service responding with HTTP 304. I think you laid out the two options pretty well.
The service will need to do some minimal computation/work to arrive at the conclusion that it can respond with HTTP 304. Does it want to have that work represented in an observability backend, that is, would it create and send a span for this HTTP request? Then it should also add a new corresponding traceresponse header (this would be your "it helps debugging the 304 cases" variant).
I am not sure I follow this part
so the content of the original response and tracing headers of the new response might be inconsistent.
Is this about the responses as observed by the client? In that case, yes, if no new traceresponse header is added to the response, the client would combine a cached resource body with a traceresponse header value from the request that provided that cached response. From my perspective, this would not be too surprising for a case where the response was cached.
That's why I think this is a design decision that either the service in question or implementors of the spec in general should make, but that the spec should not mandate.
In that case, yes, if no new traceresponse header is added to the response
You mean, if new traceresponse header ARE added to the response, the client would combine ...
So the issue is that if you do want to issue headers to "help debug the 304 cases" you might get the client confused.
I had similar issue with Server-Timing header. As you noted ("the service will need to do some minimal computation/work to arrive at the conclusion that it can respond with HTTP 304") timing of the service might be relevant even in the 304 case. But if when I issued Server-Timing header with 304, then it ended up being confusing in Chrome dev tools because I was observing the response but Server-Timing was missing some entries because it ended early. Without Server-Timing header in 304 case I could observe original timing information.
I am not sure if there is a decision to be made in the spec as you note. Personally, I decide that I will not issue headers in such case but use alternative means (entries in logs) to convey this information (both timing and spans). Hopefully debugging of 304 cases will be rare so digging in logs will not be necessary.
But probably spec should note something about this issue?
In that case, yes, if no new traceresponse header is added to the response
You mean, if new traceresponse header ARE added to the response, the client would combine ...
No, I meant what I wrote.
If the service does not set a traceresponse header, the old cached traceresponse header will be re-used, right?
But probably spec should note something about this issue?
What would you suggest the spec should note about this issue?
If the service does not set a traceresponse header, the old cached traceresponse header will be re-used, right?
OK, we are on the same page here then. I got confused by your wording "the client would combine" - it does not combine in that case anything, it just keeps using the old cached body & headers.
What would you suggest the spec should note about this issue?
Maybe something along the lines of:
Care should be taken when the server responds with 304 Not Modified HTTP response code. If the server includes traceresponse headers in the response, then those headers are combined in the client with the original cached response and might be inconsistent. If the server does not include traceresponse headers in the response, then the original traceresponse headers are observed by the client and information about the new trace is not available to the client.
I like that! And I agree, including this would improve the spec.
I would rephrase it slightly. Mostly using singular form (there is only one traceresponse header per response, not multiple).
Here goes:
Care should be taken when the server responds with 304 Not Modified HTTP response code. If the server includes a traceresponse header in the response, then that header is combined in the client with the original cached response body. If the server does not include a traceresponse header in the response, then the traceresponse header value from the original cached response is observed by the client and information about the new trace is not available to the client.
What do you think?
Would you like to create a PR for this? I can also create a PR if that is more convenient for you, but since it is your contribution, I don't want to take undue credit for it. :)
then that header is combined in the client with the original cached response body.
Not really just the body, it is also combined with all other headers for that cached response.
Would you like to create a PR for this?
Sure. Do you have a suggestion where to add it in the spec?
Not really just the body, it is also combined with all other headers for that cached response.
Right.
Sure. Do you have a suggestion where to add it in the spec?
Sorry, I failed to reply here in time. Since this is not normative, I would suggest to put it somewhere in section "5.2 Processing Model for Working with Trace Context Response Header". Since it does neither fit into 5.2.1 (Restarted Trace) nor 5.2.2 (Load Balancer Deferred Sampling), maybe adding a new subsection 5.2.3 there would make sense for this?
By the way, we also discussed this in the working group meeting yesterday and @dyladan seemed to have a slightly different opinion than me. More along the lines "maybe the spec should not strictly mandate a specific behavior but at least recommend to issue a new traceresponse header".
By the way, we also discussed this in the working group meeting yesterday and @dyladan seemed to have a slightly different opinion than me. More along the lines "maybe the spec should not strictly mandate a specific behavior but at least recommend to issue a new traceresponse header".
Hm, I see. So there is really a span of views here: mine being that the recommendation should be not to issue a new traceresponse header for 304 respones, your being that we should mention the problem and leave it to users to decide, and @dyladan's that we should recommend to issue a new header. :-)
Should I start with PR and we flesh it out there? Or?