feat: add metrics=timings preference to prefer header #3507

taimoorzaeem · 2024-05-11T09:38:01Z

Closes #3410.

Implementation
Tests
Docs

laurenceisla · 2024-05-20T22:11:47Z

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 · 2024-05-20T22:35:12Z

src/PostgREST/App.hs

+-- 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


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

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.

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.

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.

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

steve-chavez · 2025-04-30T23:56:30Z

CHANGELOG.md

@@ -57,6 +58,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   + Return new `PGRST303` error when jwt claims decoding fails
 - #3906, Return `PGRST125` and `PGRST126` errors instead of empty json - @taimoorzaeem

+### Deprecated
+
+- #3410, The config `server-timing-enabled` is deprecated. Use `metrics=timings` prefer header instead - @taimoorzaeem


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.

This allows obtaining the preferences header before doing the full parse on userApiRequest. Which is needed by PostgREST#3507.

steve-chavez · 2025-05-02T23:35:01Z

docs/references/api/preferences.rst

+.. note::
+
+  When timings are requested with this header, we can't tell the ``parse`` stage timings. This is because we are past the "parse" stage when this preference itself is parsed.


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.

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.

This allows obtaining the preferences header before doing the full parse on userApiRequest. Which is needed by #3507.

taimoorzaeem · 2025-05-03T06:40:27Z

src/PostgREST/Auth.hs

+      -- Now that we also calculate the timings when Prefer: metrics=timings,
+      -- how do we get that preference here?


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

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.

Agreed. Let me refactor this.

taimoorzaeem force-pushed the prefer/metrics branch 2 times, most recently from 953a808 to 0da1e8c Compare May 12, 2024 20:21

taimoorzaeem force-pushed the prefer/metrics branch from e5d6008 to bfd02a2 Compare May 19, 2024 19:46

laurenceisla reviewed May 20, 2024

View reviewed changes

taimoorzaeem marked this pull request as draft April 14, 2025 11:54

taimoorzaeem force-pushed the prefer/metrics branch 2 times, most recently from 57a6f25 to 00cca6c Compare April 30, 2025 18:55

taimoorzaeem changed the title ~~feat: add metrics=timings to prefer header~~ feat: add metrics=timings preference to prefer header Apr 30, 2025

taimoorzaeem marked this pull request as ready for review April 30, 2025 18:57

taimoorzaeem requested a review from steve-chavez April 30, 2025 18:59

steve-chavez reviewed Apr 30, 2025

View reviewed changes

taimoorzaeem force-pushed the prefer/metrics branch from 00cca6c to 68f42f8 Compare May 1, 2025 06:36

steve-chavez added a commit to steve-chavez/postgrest that referenced this pull request May 2, 2025

refactor: split preference parsing from userApiRequest

e4a2095

This allows obtaining the preferences header before doing the full parse on userApiRequest. Which is needed by PostgREST#3507.

steve-chavez mentioned this pull request May 2, 2025

refactor: split preference parsing from userApiRequest #4057

Merged

steve-chavez reviewed May 2, 2025

View reviewed changes

steve-chavez added a commit that referenced this pull request May 3, 2025

refactor: split preference parsing from userApiRequest

b582538

This allows obtaining the preferences header before doing the full parse on userApiRequest. Which is needed by #3507.

feat: add metrics=timings preference to prefer header

c6c1545

taimoorzaeem force-pushed the prefer/metrics branch from 68f42f8 to c6c1545 Compare May 3, 2025 06:36

taimoorzaeem marked this pull request as draft May 3, 2025 06:37

taimoorzaeem commented May 3, 2025

View reviewed changes

taimoorzaeem mentioned this pull request May 4, 2025

refactor: remove auth middleware #4059

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add metrics=timings preference to prefer header #3507

feat: add metrics=timings preference to prefer header #3507

taimoorzaeem commented May 11, 2024 •

edited

Loading

laurenceisla commented May 20, 2024 •

edited

Loading

laurenceisla May 20, 2024 •

edited

Loading

steve-chavez Nov 19, 2024

steve-chavez Nov 19, 2024

steve-chavez Apr 25, 2025

taimoorzaeem Apr 28, 2025

steve-chavez Apr 30, 2025

steve-chavez May 2, 2025

steve-chavez May 2, 2025

taimoorzaeem May 3, 2025

steve-chavez May 3, 2025

taimoorzaeem May 4, 2025

		.. note::

		When timings are requested with this header, we can't tell the ``parse`` stage timings. This is because we are past the "parse" stage when this preference itself is parsed.

		-- Now that we also calculate the timings when Prefer: metrics=timings,
		-- how do we get that preference here?

	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

feat: add metrics=timings preference to prefer header #3507

Are you sure you want to change the base?

feat: add metrics=timings preference to prefer header #3507

Conversation

taimoorzaeem commented May 11, 2024 • edited Loading

laurenceisla commented May 20, 2024 • edited Loading

laurenceisla May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taimoorzaeem commented May 11, 2024 •

edited

Loading

laurenceisla commented May 20, 2024 •

edited

Loading

laurenceisla May 20, 2024 •

edited

Loading