search

getappmap / appmap-js

Client libraries for AppMap
48 stars 17 forks source link

Scanner - Issue an advisory when cache control headers are observed but no 304 Not Modified ever is #1442

Open dustinbyrne opened 11 months ago

dustinbyrne commented 11 months ago

See the following report: https://github.com/getappmap/appmap-server/pull/1331#issuecomment-1769293435

Depending on the order tests are run, sometimes the client already has a cached response. This will change the response status from 200 to 304 in the OpenAPI diff. It's not immediately clear to me how to resolve this.

A couple of options may be:

  1. Ignore 304 responses and consider them 200 when building OpenAPI definitions
  2. Include both variants in OpenAPI definitions if an appropriate Cache-Control header is included in a server response
  3. Ignore 304 responses in an Analysis report
kgilpin commented 11 months ago

Thanks for figuring this out! Actually I think we can leave the OpenAPI behavior as is, and report an advisory when 304 is never observed in the test run.

kgilpin commented 11 months ago

We can detect the case when the server is reporting cache control headers but 304 is never actually observed (and thus may not be working). I would consider this a performance advisory.

kgilpin commented 11 months ago

Can you open an issue on the server to ensure that both 200 and 304 are observed on every test run?