User/Role/Feature lookup improvements #888

NickLaMuro · 2020-09-01T01:23:37Z

Requires ManageIQ/manageiq#20506

Makes use of new scopes to improve the number of queries required for login and even regular requests (though, the latter is less significant).

Also avoids some duplicate User lookups on login.

Benchmarks

Note: Improvements are in number of queries.

Before

$ bundle exec miqperf benchmark -ac2 "/api/users"
$ bundle exec miqperf report --last
/api/auth
|   ms | queries | query (ms) | rows |
| ---: |    ---: |       ---: | ---: |
| 5088 |      26 |        183 |   10 |
/api/users
|   ms | queries | query (ms) | rows |
| ---: |    ---: |       ---: | ---: |
|  267 |      12 |         13 | 1494 |
|   17 |      10 |        3.9 |   12 |

After

$ bundle exec miqperf benchmark -ac2 "/api/users"
$ bundle exec miqperf report --last
/api/auth
|   ms | queries | query (ms) | rows |
| ---: |    ---: |       ---: | ---: |
| 4949 |      12 |        181 |    1 |
/api/users
|   ms | queries | query (ms) | rows |
| ---: |    ---: |       ---: | ---: |
|  258 |       7 |         12 | 1489 |
|   15 |       5 |        3.3 |    7 |

Links

User login scoping (API) manageiq#20506
More improvements as part of this effort: API Performance #880

kbrock

this is simple and really nice

one question on the if else, but just a nit

We've made changes exactly like this in the past, so they follow a common pattern.
But I am surprised how much of an impact it made.
very nice find

app/controllers/api/base_controller/authentication.rb

lib/services/api/user_token_service.rb

app/controllers/api/auth_controller.rb

NickLaMuro · 2020-09-02T17:33:23Z

Looks like I have a lot of broken tests to fix... I will look into that today.

Edit: Actually, this needs a cross repo test for this to function, so will work on that at some point.

Fryguy · 2020-09-02T17:36:48Z

lib/services/api/user_token_service.rb

-      validate_userid(userid)
+    def generate_token(user_or_id, requester_type, token_ttl: nil)
+      if user_or_id.kind_of?(User)
+        userid = user_or_id.userid.downcase


do we still need to do the validation? (at least the "in my region" aspect of what the validate method does)?

I think I may have screwed up the if here, and the end should be a line earlier... let me look into this...

(sorry I took so long to respond to this...)

Use the new `:lookup_scope` argument for `.authenticate` and `.lookup_by_identity` to reduce the number of user based queries on every request.

While `virtual_delegates` can be used to improve performance via a single attribute in a query, we already bring back the `miq_user_role` (via the lookup_scope), so it is better to just use that. And currently virtual_delegates don't support already loaded relationships.

There are a few places in the API where we re-fetch the User when it is already available. These fixes allow for making use of the existing user that already exists instead of calling another lookup. The two places involved are: - When authenticating using .basic_authentication - When generating a token for the user (on login)

miq-bot · 2023-02-27T00:19:34Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot · 2023-05-29T00:00:44Z

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

Fryguy · 2023-07-27T15:38:35Z

cc @kbrock

miq-bot · 2023-07-27T15:45:25Z

Checked commits NickLaMuro/manageiq-api@d1c9e09~...db287af with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
4 files checked, 1 offense detected

app/controllers/api/base_controller/authentication.rb

❗ - Line 93, Col 63 - Style/HashSyntax - Use hash rockets syntax.

miq-bot · 2023-10-30T00:00:17Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot · 2024-02-05T00:00:19Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

miq-bot · 2024-05-06T00:00:11Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

miq-bot · 2024-08-12T00:00:15Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

miq-bot · 2024-11-25T00:00:56Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

NickLaMuro requested review from abellotti and bdunne as code owners September 1, 2020 01:23

NickLaMuro mentioned this pull request Sep 1, 2020

User login scoping (API) ManageIQ/manageiq#20506

Closed

gtanzillo mentioned this pull request Sep 1, 2020

API Performance #880

Closed

8 tasks

kbrock approved these changes Sep 1, 2020

View reviewed changes

app/controllers/api/base_controller/authentication.rb Outdated Show resolved Hide resolved

lib/services/api/user_token_service.rb Show resolved Hide resolved

kbrock reviewed Sep 1, 2020

View reviewed changes

app/controllers/api/auth_controller.rb Show resolved Hide resolved

NickLaMuro force-pushed the user-lookup-scoping-support branch from 890fe16 to f28b015 Compare September 1, 2020 16:52

Fryguy reviewed Sep 2, 2020

View reviewed changes

chessbyte added the performance label Sep 3, 2020

chessbyte assigned kbrock Sep 3, 2020

NickLaMuro mentioned this pull request Sep 28, 2020

API Performance (Round 2) #889

Open

4 tasks

NickLaMuro force-pushed the user-lookup-scoping-support branch from f28b015 to 47a8ae2 Compare January 15, 2021 21:12

NickLaMuro force-pushed the user-lookup-scoping-support branch from 47a8ae2 to cd3e8d1 Compare January 22, 2021 22:06

NickLaMuro added 2 commits January 29, 2021 11:09

[BaseController::Authentication] Use :api_includes

d1c9e09

Use the new `:lookup_scope` argument for `.authenticate` and `.lookup_by_identity` to reduce the number of user based queries on every request.

NickLaMuro force-pushed the user-lookup-scoping-support branch from cd3e8d1 to 633d396 Compare January 29, 2021 17:09

NickLaMuro force-pushed the user-lookup-scoping-support branch from 633d396 to db287af Compare January 29, 2021 17:13

miq-bot added the stale label Feb 27, 2023

miq-bot closed this May 29, 2023

Fryguy removed the stale label Jul 27, 2023

Fryguy reopened this Jul 27, 2023

miq-bot added the stale label Oct 30, 2023

kbrock removed the stale label Oct 30, 2023

miq-bot added the stale label Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User/Role/Feature lookup improvements #888

User/Role/Feature lookup improvements #888

NickLaMuro commented Sep 1, 2020

kbrock left a comment •

edited

Loading

NickLaMuro commented Sep 2, 2020 •

edited

Loading

Fryguy Sep 2, 2020

NickLaMuro Sep 11, 2020

miq-bot commented Feb 27, 2023

miq-bot commented May 29, 2023

Fryguy commented Jul 27, 2023

miq-bot commented Jul 27, 2023

miq-bot commented Oct 30, 2023

miq-bot commented Feb 5, 2024

miq-bot commented May 6, 2024

miq-bot commented Aug 12, 2024

miq-bot commented Nov 25, 2024

User/Role/Feature lookup improvements #888

Are you sure you want to change the base?

User/Role/Feature lookup improvements #888

Conversation

NickLaMuro commented Sep 1, 2020

Benchmarks

Before

After

Links

kbrock left a comment • edited Loading

Choose a reason for hiding this comment

NickLaMuro commented Sep 2, 2020 • edited Loading

Fryguy Sep 2, 2020

Choose a reason for hiding this comment

NickLaMuro Sep 11, 2020

Choose a reason for hiding this comment

miq-bot commented Feb 27, 2023

miq-bot commented May 29, 2023

Fryguy commented Jul 27, 2023

miq-bot commented Jul 27, 2023

miq-bot commented Oct 30, 2023

miq-bot commented Feb 5, 2024

miq-bot commented May 6, 2024

miq-bot commented Aug 12, 2024

miq-bot commented Nov 25, 2024

kbrock left a comment •

edited

Loading

NickLaMuro commented Sep 2, 2020 •

edited

Loading