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

Should tracing headers be included in 304 Not Modified responses? #554

Open
mitar opened this issue Nov 13, 2023 · 14 comments
Open

Should tracing headers be included in 304 Not Modified responses? #554

mitar opened this issue Nov 13, 2023 · 14 comments

Comments

@mitar
Copy link

mitar commented Nov 13, 2023

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.
@yurishkuro
Copy link
Member

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.

@mitar
Copy link
Author

mitar commented Nov 13, 2023

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.

@yurishkuro
Copy link
Member

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.

@mitar
Copy link
Author

mitar commented Nov 13, 2023

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 Upgrade

All 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.

@kalyanaj
Copy link
Contributor

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.

@mitar
Copy link
Author

mitar commented Nov 14, 2023

Yes, sorry. This is about response headers.

@basti1302
Copy link
Contributor

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.

@mitar
Copy link
Author

mitar commented Nov 21, 2023

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?

@basti1302
Copy link
Contributor

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?

@mitar
Copy link
Author

mitar commented Nov 24, 2023

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.

@basti1302
Copy link
Contributor

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. :)

@mitar
Copy link
Author

mitar commented Nov 24, 2023

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?

@basti1302
Copy link
Contributor

basti1302 commented Dec 6, 2023

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".

@mitar
Copy link
Author

mitar commented Jan 3, 2024

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?

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

No branches or pull requests

4 participants