Added a Bull client, and a route /feeds/info to query queue's info #2541

TueeNguyen · 2021-11-28T23:20:42Z

Issue This PR Addresses

This fixes #2414

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

This is draft PR for issue #2414.

I added a new file queue.js that creates and exports a new Bull client with redis client connection required from posts/src/redis.js. I also added a new GET route feeds/info to return the queue's information, for now I'm trying to return the number of jobs in the queue.

I tested the route by running npm run services:start, npm start first and running http://localhost/v1/posts/feeds/info in Postman. However, I haven't got the expected response, please let me know if I'm on the right track!

The response I got was a long loading screen until Bad Gateway

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

gitpod-io · 2021-11-28T23:20:45Z

humphd

You should try running this in GitPod so you don't have to set everything up locally.

Also, you have some lint errors to fix.

humphd · 2021-11-29T00:55:55Z

src/api/posts/src/routes/feeds.js

+    logger.error({ error }, "Unable to get queue's info");
+    next(error);
+  }
+  res.json({ jobNum: jobNum });


Move this to after line 42, inside the try/catch. Also, you don't need to repeat jobNum:

res.json({ jobNum });

humphd · 2021-11-29T00:56:23Z

src/api/posts/src/routes/feeds.js

@@ -42,7 +55,7 @@ feeds.get('/:id', validateFeedsIdParam(), async (req, res, next) => {
    const feed = await Feed.byId(id);
    if (!feed) {
      res.status(404).json({
-        message: `Feed not found for id ${id}`,
+        message: `Feed not found fore id ${id}`,


Spelling mistake. Should be for not fore.

humphd · 2021-11-29T03:02:58Z

src/api/posts/src/routes/feeds.js

+feeds.get('/info', async (req, res, next) => {
+  let jobNum;
+  try {
+    jobNum = await queue.count();


Tested on GitPod and this route never seems to return (e.g., I get no response from the server, it just loads forever). It looks like you're doing it correctly, but there's nothing coming back.

TueeNguyen · 2021-11-29T03:42:24Z

That's what I got for my response as well, the idea seems correct though. I'll look more into it

humphd · 2021-11-29T13:53:45Z

I suspect that the connection to Redis isn't right somehow (e.g., it's never returning a response).

TueeNguyen · 2021-11-29T23:22:23Z

I'll compare it to the connection in backend. Is it possible to log the connection using logger?

TueeNguyen · 2021-12-01T05:56:29Z

Hi @humphd, it's not returning any data because the redis clients from backend and posts have different host.

The Redis_URL in backend is redis:://127.0.0.1 while the one from satellite is redis://redis:6379

const redisUrl = process.env.REDIS_URL || 'redis://redis:6379'; // redis.js from satellite

telescope/src/backend/lib/redis.js

Line 9 in 2359565

    
           parseUrl(process.env.REDIS_URL, process.env.REDIS_PORT) || 'redis://127.0.0.1:6379';

I tried uncommenting this line #REDIS_URL=redis://127.0.0.1 from env.development and building the containers again using docker-compose --env-file ./config/env.development up --build but the host remain the same.

How do I change the variable REDIS_URL to redis:://127.0.0.1 for satellite's redis.js file

humphd · 2021-12-01T14:58:39Z

Fantastic detective work! I'm really impressed that you didn't give up, and kept pushing to find the underlying problem. That's awesome.

We should change the backend to match the microservices, unless @manekenpix or @HyperTHD have a better idea?

NOTE: all the config happens via environment variables in the *.env files https://github.com/Seneca-CDOT/telescope/tree/master/config, so that's:

https://github.com/Seneca-CDOT/telescope/blob/master/config/env.development#L225-L227

We also override things in the docker-compose* files in https://github.com/Seneca-CDOT/telescope/blob/master/docker. For example, I think we should probably change this to use the docker network instead of exposing:

https://github.com/Seneca-CDOT/telescope/blob/master/docker/development.yml#L72-L74

I'd have to sit with this for a while to wrap my head around it, but in general, I think the backend should be bent to work the same way as the stuff in src/api with Satellite. We're going to be getting rid of it in upcoming releases.

TueeNguyen · 2021-12-01T22:59:35Z

Thank you! I tested it out and the queue from api successfully returned stats data.

If cleaning up the backend is on the list for the next release, it'll be probably late for my release 0.4. Also, I don't think I should touch the configuration of backend in this PR (maybe in another PR).

So how should I move forward from here, should I wait for the other devs' ideas?

manekenpix · 2021-12-01T23:01:02Z

I'm not sure if this is a redis issue since all the other endpoints in api/posts rely on the same redis client used in the feeds/info endpoint, and they all can connect and retrieve data from redis.

humphd · 2021-12-01T23:01:46Z

I agree it's odd that this isn't working, since everything else is running against that same Redis instance. Is it just your dev setup?

TueeNguyen · 2021-12-02T02:20:57Z

I agree, if it works for other endpoints why not info?

It's just when I followed Josue's instructions, the promise from queue.count() got resolved once, when /info was hit, it still didn't return nothing. I thought changing the .env would solve the problem.

I'll keep trying.

TueeNguyen · 2021-12-02T17:21:17Z

Turns out, I need to provide the createClient with type client or subscriber for queues to share the same connection according to the documentation.
This is the response I've got for now, completed is also 0

{"queueInfo":{"waiting":785,"active":0,"completed":0,"failed":2,"delayed":0,"paused":0,"jobCnt":785}}

The reason completed is 0 is because of the 2 properties below are true.

telescope/src/backend/feed/queue.js

Lines 29 to 30 in 4b1c074

    
           removeOnComplete: true, 
        
           removeOnFail: true,

I also added the 2 options const redis = Redis({ maxRetriesPerRequest: null, enableReadyCheck: false }); in redis.js because I ran into this problem when building the containers. The CHANGELOG from Bull also mentions this. Seems like it didn't pass the test

humphd

We need to add a test for this as well in https://github.com/Seneca-CDOT/telescope/blob/master/src/api/posts/test/posts.test.js. We won't have real data, but hopefully it should return something.

I'm fine if you do that test here or in a follow-up PR. If you do the latter, please file the issue now.

src/api/posts/src/redis.js

src/api/posts/src/routes/feeds.js

TueeNguyen · 2021-12-03T05:51:06Z

We need to add a test for this as well in https://github.com/Seneca-CDOT/telescope/blob/master/src/api/posts/test/posts.test.js. We won't have real data, but hopefully it should return something.

I'm fine if you do that test here or in a follow-up PR. If you do the latter, please file the issue now.

I'll add tests in this PR

TueeNguyen · 2021-12-04T00:48:25Z

Hi, for now, waiting, active, delayed, jobCnt of the response updates on changes of the queue. completed, failed are always 0 as completed and failed jobs are removed from the queue.

{"queueInfo":{"waiting":785,"active":0,"completed":0,"failed":2,"delayed":0,"paused":0,"jobCnt":785}}

I try listening for completed queue events but the event is never fired as the queue in post is not consumer or producer. I also try adding 'global:completed', the event is fired and I've got expected response but it returns error when I try to use getJobcounts() on the queue. The error is "Connection in subscriber mode, only subscriber commands may be used"

//this event is never fired
queue.on('completed', (job) => {
  console.log(`${job.id} completed`);
});
//this event is fired but get error when trying to use Bull Api on queue
queue.on('global:completed', (jobid) => {
  console.log(`${jobid} completed`);
});

I'm not sure how to get completed and active to update. I need some help on this
Helpful doc links: bull, Global Events

humphd · 2021-12-04T00:50:36Z

This is one reason why doing this in the parser would make the most sense, since that's where we actually manage the queue to begin with.

That said, why don't we start with the items you can get, and do other things in follow ups?

TueeNguyen · 2021-12-04T00:54:30Z

If that's the case, I'd love to work on the parser service later on.

I'll follow up with what I can get right now. Thanks.

TueeNguyen · 2021-12-06T18:19:29Z

I clicked on Update Branch accidentally.

humphd · 2021-12-06T18:23:52Z

git checkout -B issue-2414 2fd0b98b1090925ca2869cf7a3dbabb97cf770fc
git checkout master
git pull upstream master
git checkout issue-2414
git rebase master
git push origin issue-2414 -f

TueeNguyen · 2021-12-06T21:11:54Z

After this got merged, I need to work on the front-end right?

humphd · 2021-12-06T22:09:33Z

Yup, you can start now if you want, just base your new branch on this branch, and rebase onto master when it lands.

TueeNguyen · 2021-12-06T22:10:45Z

Got it!

HyperTHD

Just for reference, Redis was added to Satellite so we should extract it from there. That way, we can also remove ioredis and ioredis-mock from the posts service. We can do that in another issue though. Nice job

humphd · 2021-12-10T14:08:05Z

@TueNguyen2911 let's get this in soon, can you rebase and fix the conflict in your package.json? Ping me when it's ready so I can merge.

humphd · 2021-12-10T19:17:50Z

src/api/posts/package.json

@@ -17,6 +17,7 @@
  },
  "dependencies": {
    "@senecacdot/satellite": "^1.x",
+    "bull": "^3.20.1",


Because you a dependency, you need to also update the pnpm lock file and commit that as well.

- Added a test for the new route - Added Bull to package.json - Added a new file queue.js to create a Bull queue - Updated pnpm-lock.yaml

vercel bot deployed to Preview November 28, 2021 23:20 View deployment

humphd requested changes Nov 29, 2021

View reviewed changes

vercel bot deployed to Preview November 29, 2021 02:28 View deployment

humphd requested changes Nov 29, 2021

View reviewed changes

manekenpix assigned TueeNguyen Dec 1, 2021

TueeNguyen force-pushed the issue-2414 branch from deb8e03 to 0ffda02 Compare December 2, 2021 17:07

vercel bot deployed to Preview December 2, 2021 17:07 View deployment

vercel bot deployed to Preview December 2, 2021 17:12 View deployment

humphd requested changes Dec 2, 2021

View reviewed changes

src/api/posts/src/redis.js Outdated Show resolved Hide resolved

src/api/posts/src/routes/feeds.js Outdated Show resolved Hide resolved

vercel bot deployed to Preview December 3, 2021 05:48 View deployment

vercel bot deployed to Preview December 3, 2021 05:50 View deployment

vercel bot deployed to Preview December 4, 2021 22:13 View deployment

TueeNguyen force-pushed the issue-2414 branch from c00e7bc to 71344c0 Compare December 4, 2021 22:34

vercel bot deployed to Preview December 4, 2021 22:34 View deployment

humphd marked this pull request as ready for review December 5, 2021 03:35

humphd previously approved these changes Dec 5, 2021

View reviewed changes

TueeNguyen dismissed humphd’s stale review via 2fd0b98 December 6, 2021 18:11

TueeNguyen force-pushed the issue-2414 branch from 71344c0 to 2fd0b98 Compare December 6, 2021 18:11

vercel bot deployed to Preview December 6, 2021 18:11 View deployment

vercel bot deployed to Preview December 6, 2021 18:16 View deployment

humphd previously approved these changes Dec 6, 2021

View reviewed changes

TueeNguyen dismissed humphd’s stale review via 7f190d8 December 6, 2021 18:27

TueeNguyen force-pushed the issue-2414 branch from 872ca06 to 7f190d8 Compare December 6, 2021 18:27

vercel bot deployed to Preview December 6, 2021 18:27 View deployment

humphd previously approved these changes Dec 6, 2021

View reviewed changes

HyperTHD previously approved these changes Dec 6, 2021

View reviewed changes

TueeNguyen mentioned this pull request Dec 8, 2021

Added jobs count to dashboard #2585

Merged

8 tasks

TueeNguyen dismissed stale reviews from HyperTHD and humphd via 881e9cc December 10, 2021 19:16

TueeNguyen force-pushed the issue-2414 branch from 7f190d8 to 881e9cc Compare December 10, 2021 19:16

vercel bot deployed to Preview December 10, 2021 19:16 View deployment

humphd requested changes Dec 10, 2021

View reviewed changes

- Added a new GET route /feeds/info returning queue stats

3409f74

- Added a test for the new route - Added Bull to package.json - Added a new file queue.js to create a Bull queue - Updated pnpm-lock.yaml

TueeNguyen force-pushed the issue-2414 branch from 881e9cc to 3409f74 Compare December 10, 2021 19:21

vercel bot deployed to Preview December 10, 2021 19:21 View deployment

humphd merged commit 5730360 into Seneca-CDOT:master Dec 10, 2021

Added a Bull client, and a route /feeds/info to query queue's info #2541

Added a Bull client, and a route /feeds/info to query queue's info #2541

TueeNguyen commented Nov 28, 2021 •

edited

gitpod-io bot commented Nov 28, 2021

humphd left a comment

humphd Nov 29, 2021

humphd Nov 29, 2021

humphd Nov 29, 2021

TueeNguyen commented Nov 29, 2021

humphd commented Nov 29, 2021

TueeNguyen commented Nov 29, 2021

TueeNguyen commented Dec 1, 2021

humphd commented Dec 1, 2021

TueeNguyen commented Dec 1, 2021

manekenpix commented Dec 1, 2021

humphd commented Dec 1, 2021

TueeNguyen commented Dec 2, 2021 •

edited

TueeNguyen commented Dec 2, 2021 •

edited

humphd left a comment

TueeNguyen commented Dec 3, 2021

TueeNguyen commented Dec 4, 2021

humphd commented Dec 4, 2021

TueeNguyen commented Dec 4, 2021

TueeNguyen commented Dec 6, 2021

humphd commented Dec 6, 2021

TueeNguyen commented Dec 6, 2021

humphd commented Dec 6, 2021

TueeNguyen commented Dec 6, 2021

HyperTHD left a comment

humphd commented Dec 10, 2021

humphd Dec 10, 2021

Added a Bull client, and a route /feeds/info to query queue's info #2541

Added a Bull client, and a route /feeds/info to query queue's info #2541

Conversation

TueeNguyen commented Nov 28, 2021 • edited

Issue This PR Addresses

Type of Change

Description

Checklist

gitpod-io bot commented Nov 28, 2021

humphd left a comment

Choose a reason for hiding this comment

humphd Nov 29, 2021

Choose a reason for hiding this comment

humphd Nov 29, 2021

Choose a reason for hiding this comment

humphd Nov 29, 2021

Choose a reason for hiding this comment

TueeNguyen commented Nov 29, 2021

humphd commented Nov 29, 2021

TueeNguyen commented Nov 29, 2021

TueeNguyen commented Dec 1, 2021

humphd commented Dec 1, 2021

TueeNguyen commented Dec 1, 2021

manekenpix commented Dec 1, 2021

humphd commented Dec 1, 2021

TueeNguyen commented Dec 2, 2021 • edited

TueeNguyen commented Dec 2, 2021 • edited

humphd left a comment

Choose a reason for hiding this comment

TueeNguyen commented Dec 3, 2021

TueeNguyen commented Dec 4, 2021

humphd commented Dec 4, 2021

TueeNguyen commented Dec 4, 2021

TueeNguyen commented Dec 6, 2021

humphd commented Dec 6, 2021

TueeNguyen commented Dec 6, 2021

humphd commented Dec 6, 2021

TueeNguyen commented Dec 6, 2021

HyperTHD left a comment

Choose a reason for hiding this comment

humphd commented Dec 10, 2021

humphd Dec 10, 2021

Choose a reason for hiding this comment

TueeNguyen commented Nov 28, 2021 •

edited

TueeNguyen commented Dec 2, 2021 •

edited

TueeNguyen commented Dec 2, 2021 •

edited