feat: add metrics=timings preference to prefer header

[taimoorzaeem]

Closes #3410.

• Implementation
• Tests
• Docs

[laurenceisla]

What exactly do I need to do to deprecate the server-timings-enabled config? Should I just remove it from the codebase?

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

[laurenceisla]

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

[steve-chavez]

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.

[steve-chavez]

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.

[steve-chavez]

I was thinking to do a breaking change and remove the parse time now. But then I thought: wouldn't it be simpler to omit the parse time when Prefer: metric=timings is requested?

It would just be:

Server-Timing: jwt;dur=14.9, plan;dur=109.0, transaction;dur=353.2, response;dur=4.4

We could point that out in the docs.

@taimoorzaeem I believe that should unblock this feature.

[taimoorzaeem]

Yes, that would be much simpler. Let's implement it this way.

[steve-chavez]

Now that server-timing-enabled has different functionality than Prefer: metrics=timings, I don't think we can deprecate it/remove it in the future. At least for now.

[steve-chavez]

This didn't look good. So I thought of a simpler way: #4057.

With this, we won't measure the parse time of the Preferences header, but that's fine as its parsing is simple and its input small.

The bulk of the parse stage is really in QueryParams.parse (URL query string parsing) which is still maintained.

[steve-chavez]

To be clear, once you rebase on top of #4057, the Prefer: metric=timings resposne now should contain the same information as when the server-timing-enabled config is set.

[taimoorzaeem]

@steve-chavez We need the preference here too. I am not sure about this but I think we should think about storing the ApiRequest result in AppState. This way it would be easier to access already parsed request information at any stage in the code.

[steve-chavez]

I think we should just refactor to remove the Auth.middleware approach. I've never liked that one as it hides side effects and obscures logic. It might allow us to remove the unsafePerformIO from Auth.hs too:

postgrest/src/PostgREST/Auth.hs

Lines 190 to 198 in e4a2095

	authResultKey :: Vault.Key (Either Error AuthResult)
	authResultKey = unsafePerformIO Vault.newKey
	{-# NOINLINE authResultKey #-}
	getResult :: Wai.Request -> Maybe (Either Error AuthResult)
	getResult = Vault.lookup authResultKey . Wai.vault
	jwtDurKey :: Vault.Key Double
	jwtDurKey = unsafePerformIO Vault.newKey

Once that's done you should be able to use/get the prefer value value before validating the JWT.

[taimoorzaeem]

Agreed. Let me refactor this.