feat: add metrics=timings to prefer header #3507
Conversation
taimoorzaeem
force-pushed
the
prefer/metrics
branch
2 times, most recently
from
953a808
to
0da1e8c
Compare
taimoorzaeem
force-pushed
the
prefer/metrics
branch
from
e5d6008
to
bfd02a2
Compare
Deprecating the config does not mean to remove it from the code (right now), only that it's not recommended anymore (could be removed in the future). It should be mentioned in comments and in the docs that it's deprecated in favor of the new feature. See, for example: 6c3d7a9 and https://postgrest.org/en/latest/references/api/preferences.html#single-json-object-as-function-parameter |
| -- the preference metrics=timings cant be used before it is parsed, hence | ||
| -- parseTime will be calculated for all requests | ||
| (parseTime, apiReq@ApiRequest{..}) <- withTiming True $ liftEither . mapLeft Error.ApiRequestError $ ApiRequest.userApiRequest conf req body sCache |
There was a problem hiding this comment.
Ah, so it's not possible to disable ALL timings with Prefer: metrics=timings, without parsing the headers first. So it's not a perfect replacement for server-timings-enabled = false, since some timings will be calculated even if the header is not sent: the "jwt" and "parse" timings as far as I can see.
Not sure if it's OK to always calculate these timings, I believe it will still affect the performance as mentioned in the issue. cc: @steve-chavez
There was a problem hiding this comment.
Hm, so here we're forcing to always calculate the time for parsing? That would defeat the whole purpose of #3410
But I understand that we first need to parse to get the header. I guess we would need to do this in a step separate from userApiRequest, but that's messy.
There was a problem hiding this comment.
So far the parse and response timings (ref) have not been that useful because they're always fast.
Another option could be to omit the parse timing when doing Prefer: metrics=timings or we could just remove support for the above mentioned. Then we wouldn't have this problem.
Closes #3410.
@steve-chavez @wolfgangwalther What exactly do I need to do to deprecate the
server-timings-enabledconfig? Should I just remove it from the codebase?