Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve global request performances #354

Merged

Conversation

quentinguidee
Copy link
Contributor

@quentinguidee quentinguidee commented Feb 29, 2024

  • Improve some requests performances (ranking, artists, tracks, albums, listening time...)
  • Adds a new hidden page to test the requests

Benchmarks: #354 (comment)

@Yooooomi
Copy link
Owner

I don't know what you think about the userMetric thing in the app. I think it's not available anymore but it used to allow people to choose between choosing from "Count" or "Duration". It makes frontend more complex and requests more complex too so maybe we should stick to one or always do both.
I'd be curious to know what you think :)

@quentinguidee
Copy link
Contributor Author

quentinguidee commented Feb 29, 2024

Actually in my opinion that's something to consider and to keep since both are useful and different statistics. For now for this PR I'll keep the requests as they are, because each deep modifications of the requests should probably have their own PR and discussions. I'll dedicate this PR to late $lookup first!

Also, I think that this filter can be more pertinent in places like /top/tracks.

Ultimately, the /top/tracks route might also be better as just /tracks with filters/sort capabilities:

  • /tracks Get the tracks, probably with a default sort of last listening date
  • /tracks?sort=listening_count Get the top tracks
  • /tracks?sort=listening_count&desc=1 Get least listened tracks
  • /tracks?genre=abc&sort=listening_count etc...

I think this approach might be is more RESTful

@Yooooomi
Copy link
Owner

Yeah I agree with you but in the end they define a use case that has to be present in the app. I feel like query flexibility should be linked to frontend graph flexibility. As of today you cannot customize graphs much so there's not real need for that.

@quentinguidee
Copy link
Contributor Author

For this specific frontend yep, but anyone that want to use the API for its own frontend can't really use it except if it reproduces the exact same views. If I want to make a small utility or script that uses my stored listening history, I cannot do that without editing the server (except if the queries are used by the yourspotify frontent).

@Yooooomi
Copy link
Owner

Yooooomi commented Mar 1, 2024

Ok yeah I can understand, I just feel like we should put minimal effort on this on our way to performances.

@quentinguidee
Copy link
Contributor Author

quentinguidee commented Mar 1, 2024

Artists/Albums/Tracks pages and ranking are instantaneous now 🚀 That's really cool to navigate

@quentinguidee quentinguidee marked this pull request as ready for review March 1, 2024 16:00
@Yooooomi
Copy link
Owner

Yooooomi commented Mar 1, 2024

Hey, really nice work! However I can't seem to find the functions getRank, getBest and the itemTypes enum? Is there anything I'm missing?

Comment on lines 15 to 31
export type ItemType = {
field: string;
};

export const itemTypes: {
[key in "track" | "album" | "artist"]: ItemType;
} = {
track: {
field: "$id",
},
album: {
field: "$albumId",
},
artist: {
field: "$primaryArtistId",
},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is here! Idk why github is collapsing this file in the diff

Signed-off-by: Quentin Guidée <[email protected]>
@quentinguidee
Copy link
Contributor Author

quentinguidee commented Mar 1, 2024

I added this hidden page so we can benchmark more easily requests separately. So that's the current state:

Before this PR

image

After

image

Also, requests for artists/tracks/albums are not shown yet

Signed-off-by: Quentin Guidée <[email protected]>
@Yooooomi
Copy link
Owner

Yooooomi commented Mar 1, 2024

Looks awesome, I love stats and numbers haha! Do the requests follow the interval given on top? Are you ready for a merge?

@quentinguidee
Copy link
Contributor Author

Yeah the intervals are from the header!

Yep it's ready. There are still improvements that we can do but that'll be enough for this one!

@Yooooomi Yooooomi changed the base branch from master to perf-improvements March 1, 2024 19:27
@Yooooomi Yooooomi merged commit 199aedc into Yooooomi:perf-improvements Mar 1, 2024
@Yooooomi
Copy link
Owner

Yooooomi commented Mar 1, 2024

Hahaha
image
I'm fixing that dont worry

@Yooooomi
Copy link
Owner

Yooooomi commented Mar 1, 2024

Are you working on anything specific right now? I feel like I could give some requests a try too.

@quentinguidee
Copy link
Contributor Author

No, everything for now was in this pr 👍

@quentinguidee
Copy link
Contributor Author

Also after this I think you can mark #162, #232 and #323 as resolved. There are also a lot of issues that could be closed

@Yooooomi
Copy link
Owner

Yooooomi commented Mar 1, 2024

You inspired me #355 haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants