search

Seklfreak / Robyul2

🤖 The new bot for the I.O.I discord and some other (Kpop) discords!
https://robyul.chat/
GNU Affero General Public License v3.0
63 stars 18 forks source link

YouTube Module #2

Closed Seklfreak closed 6 years ago

Seklfreak commented 7 years ago
chanyoung commented 7 years ago

Hi, Sebastian.

I'm working on the last feature of this issue: "show information about a channel or video" at youtube-search branch in my fork repository. I've tried to follow the guideline here and I think the prototype has complete. So I want to get reviews from you before keep going on.

I'll attach some operating pictures here. This is just a prototype, any kind of comments will be helpful and welcome :)

test1

test2

test4

test5

test6

test7

test8

test9

Thanks, CPark

Seklfreak commented 7 years ago

Hello CPark,

Thanks for your work! Looks great.

The YouTube API has a fairly high request limit each day, so I think two requests per command is fine.

Regarding the posting: I don't think posting two messages for each video is a good solution. It can make the flow confusing on high activity servers and also consumes more Discord API requests, which can be an issue. My idea would be to just do the Robyul embed with the video information. You can display the thumbnail there and link the video.

How about a different command that displays just the link to the video? Or a sub command, maybe _yt video <video name>?

You should also allow optional url parameters in the YouTube Regular Expression to prevent this:

My last suggestion would be to implement proper reformatting for the Published at format to make it readable. :)

Greetings, Sebastian

chanyoung commented 7 years ago

Hi Sebastian,

Thanks for your good review and suggestions.

I agree posting two messages for each video is not good solution. My main concern was 'Is this plugin has to post message with EmbedVideo or not?' If send thumbnail image and link the video would be enough, then that single post message is the best solution.

I think subcommand or new command for displays just the link to the video should be discussed after when youtube module is finished. I think it would be better to focus on here's milestone. :D

I changed several features of code.

Attach some photos. Anything needs to be fixed or changed, please let me know :) You can see codes here

1

2

3

Thanks, CPark

Seklfreak commented 7 years ago

Hey CPark,

wow so well done, amazing!

One suggestion would be ditching _yt service stop and _yt service start and just keeping _yt service restart as that does everything we could need if youtube stops working.

And I think it would sound better to replace Youtube service is not available now, please contact to manager. with YouTube service is not available right now, please contact the bot owner..

Last thing would be the channel statistics. Gugudan should have some subscribers? Is the API not working or is there an issue with the Bot? And maybe we can hide Channel Comments if they are zero, because some channels like Gugudan have channel comments disabled.

Thanks so much, Sebastian

chanyoung commented 7 years ago

Hi Sebastian,

I found that youtube channels can also hide their subscribers counts too. I think Gugudan hides their subscribers count because I couldn't find subscriber's count anywhere in their channel page. In other channel pages it works well. So I adapt same solution as comments count to subscribers count, if subscribers count is 0, then server will not includes subscribers fields in messageEmbedFields.

I removed two subcommands "service stop" and "service start", and fixes bot message as you said.

And I did a couple of optimization and code cleaning job.

I'll make a PR to your repository, let me know if something needs to be changed. You better test it enough. :D

Thanks, CPark

Seklfreak commented 7 years ago

merged and added running tests to travis, thanks, great addition!

chanyoung commented 7 years ago

Hi Sebastian,

Could you explain more details about second bullet ?

notify when view milestones are reached
  [p]youtube video add <video id/link> <discord channel>
  [p]youtube video delete <video id/link> <discord channel>
  [p]youtube video list
  notification loop

This is what I understood so far. If I add a video, then Robyul checks view counts periodically. And if view counts of that video surpass some counts(e.g., 10, 100, 1000...), then Robyul notify to channel.

I'm not sure I understood correctly..

Thanks! CPark

Seklfreak commented 7 years ago

Hello CPark,

yeah that's exactly right. YouTube Views milestones can be very important to fans that's why this would be great. I'm not sure if the view milestones (eg 10, 100, 1000, ...) should be predefined, or if they should be added while adding the YouTube video ([p]youtube video add <video id/link> <discord channel> <number of views>). Maybe you have any idea what would be best here?

Once reached the threshold Robyul should post an embed in the previously given channel showing video information and a message saying reached milestone xyz. If the list of milestones is predefined the video should stay in the database to be checked for the next milestone. If it's user defined the video can be removed from the Robyul database after posting the milestone.

Greetings, Sebastian

chanyoung commented 7 years ago

Then how about this?

[p] youtube video add <video id/link> <discord channel> <number of views> <notify when every x counts reached>

For example,

_yt video add VIDEO_A #CHANNEL_A 10000 1000

Adds 'VIDEO_A' and notify at every 1,000 view counts reached, until it meets 10,000 view counts. This will notify when view counts of 'VIDEO_A' reached 1,000 -> 2,000 -> 3,000 -> ... -> 10,000. When reached to 10,000 then delete it from database.

If user doesn't gave last argument, then just notify once when view counts reached 10,000.

Seklfreak commented 7 years ago

I love that idea, that way you can easily specify multiple steps or just one goal, I would love to see that! :)

chanyoung commented 7 years ago

Hi Sebastian!

I'm testing youtube channel feeds features and need your opinion.

I can use two kinds of API calls for checking new videos in channel. One is 'activity' call and the other is 'search' call.

I decided to use 'activity' call since 'activity' call is much cheaper than 'search' call. 'activity' call consumes 5 quota cost per request while 'search' call consumes 100 quota cost.

So I implemented sample code and testing for few days, but I found there is a problem of 'activity' call. 'activity' call can't retrieve live streaming contents, only can get uploaded contents id. However if use 'search' call, it can retrieves both of them.

Long to short, 'search' call is 20 times expensive than 'activity' call, but it can retrieve live streaming contents.

What's your choice of this?

Seklfreak commented 7 years ago

Hello! Thanks for asking about my opinion.

YouTube's API rate limits are fairly high, but 100 quote cost calls really sum up.

The YouTube API allows 300 000 queries per 100 seconds, and 1 000 000 per day. (That's quote cost)

Using the search API that's: 30 requests per second (per 100 seconds) 0.12 requests per second (daily limit) activity API: 600 requests per second (per 100 seconds) 2.31 requests per second (daily limit)

Robyul is built for KPop Servers. KPop Groups don't really use YouTube livestream, they usually use V Live. Users are more interested in getting the Videos as fast as possible (especially nice for MVs). That's why I would say choosing the activity call is the right choice, with a fast check rate.

If we could check three channels using activity in two seconds that would be really nice and leave enough space for the rather expensive calls the bot commands, such as _yt gugudan, do, I think.

chanyoung commented 7 years ago

Okay, I think activity call is more reasonable choice too.

Thanks for answering and I'll leave a message here if I have more design issues.

chanyoung commented 6 years ago

Hello Sebastian,

I finished the functionality implementation of youtube channel feeds. I'm currently doing some refactoring and testing.

I tried to design checking youtube channels as fast as possible, as you said. The check rate depends on many things. It depends on number of left quota, number of monitoring channel, time remaining until quota reset. I made check rate will change dynamically as follow.

example) A (time remaining until quota reset time) : 3,000 sec B (number of monitoring channel): 100 channels C (left quota): 100,000 quota D (youtube activity call cost): 5 quota

time interval between checking = A / ( C / (B * D) ) = 15 sec

Then every channel will be checked in 15sec period.

This logic pretty working well in my testing environments. However the problem is, I assumed youtube module is the only module which consumes youtube quota costs. Is there any other modules consume youtube quota costs?

Seklfreak commented 6 years ago

Hey CPark,

that sounds like a pretty well thought-out solution. 👍 The YouTube module is the only module that consumes the youtube api, so there shouldn't be any issues.

I'm looking forward to merging the module. :)

Seklfreak commented 6 years ago

I reviewed the pull requests! I can't thank you enough.

Some stuff that should be addressed in the future, but aren't necessary for the merge:

Also something to discuss for the future, no change needed here. You decided to store all data in one database, and used interface{}. I believe it would have been better to split that up. One table for channels, one for videos, and we could have stored the quota in redis using a few keys. I believe that would have been safer than using interface{}. Any opinions on this? I don't think it's something we should change for this module, but something to keep in mind for future development.

✨

chanyoung commented 6 years ago

I appreciate for your detailed review, I will change code to meet your requests.

I had to discuss more with you before making PR, my bad.

1) I thought one table for one module, and have an entry for module's name is the design of this program(I saw some other modules) so I made to store all types of data in a single table. I agree split table makes things easier and safer.

2) I didn't bundle requests because it's implementation complexity is higher(e.g., only some channels were succeed to post feeds by discord error) and I wasn't sure there are many cases such like multiple channels subscribing same youtube channel. However, I thought bundling of requests is necessary if there are many N to 1 cases. Are there many cases such like that?

I think this is the best time to change it, before launching new service to real users and no need to touch stored data. Would you like to defer merging PR until I make changes for it?

I will work about metrics for statistics pages after this PR merged.

Thanks :)

Seklfreak commented 6 years ago

Very good, thanks for acting so fast!

  1. That explains it. It's true most modules just use one table, but that's because they don't require more. (Also, some, especially old modules, don't exactly follow best practices :/ ) It's good that you changed it. 👍
  2. I think there will be many cases. We have individual group servers subscribing to their artists channel, and general kpop servers which might subscribe to a lot of channels. What I did for some other modules was Grouping the Database response (=> all set up feeds) by the Channel to look up. Then you can go through each channel, get the latest content, and then go through each feed entry of a channel and post the stuff.

There is no hurry to merge it and I have to test it a lot anyway, so I don't mind deferring the PR. :)

The metrics stuff is a quick change, and can be done whenever. 👍

I'm going to test the changes later today and tell you if there are any issues. :)

chanyoung commented 6 years ago

I see, bundling requests is necessary.

I will change it :)

Seklfreak commented 6 years ago

Another nice thing to add would be to initialise the youtube_posted_videos column when adding a channel with the videos from the last hour? Right now it will post all videos from last hour after adding. This can get confusing when adding channels with high activity. (for example adding Mnet during an upload session.)