Sam/marketing api#77
Conversation
swansontec
left a comment
swansontec
left a comment
There was a problem hiding this comment.
I didn't review the documentation flood, so I assume we'll drop that from the PR.
|
|
||
| ## RabbitMQ Setup (Docker) | ||
|
|
||
| **Important**: The Edge Push Server requires RabbitMQ version 3.12 with management interface. |
There was a problem hiding this comment.
I don't think this is important, since any version will do. We just need the AMQP 0.9.1 protocol, and really any implementation would be fine. Doesn't even need to be RabbitMQ, and it certainly doesn't need to be the Dockerized version.
Could we have the agent tone this down a bit? The docs mention this exact version RabbitMQ version many, many times, and it's really not necessary to be so picky.
| Monitor your AMQP queue health: | ||
|
|
||
| - **Queue depth**: Messages waiting to be processed | ||
| - **Consumer count**: Should match number of publish daemons | ||
| - **Message rates**: Publishing vs consuming rates | ||
| - **Connection status**: Watch for disconnections | ||
|
|
||
| RabbitMQ Management UI (if enabled): `http://localhost:15672` |
There was a problem hiding this comment.
So Edge actually has our RabbitMQ instance configured with Prometheus metrics. I don't remember the details of how we turned that on (probably some config file), but we ultimately proxy those through Caddy and they end up at https://push-x.edge.app:443/metrics/rabbitmq-somesecretstuff. This allows us to see the queue depth very nicely in our normal Grafana dashboard.
|
|
||
| // Databases: | ||
| amqpUri: asOptional(asString, 'amqp://username:password@localhost:5672'), | ||
| amqpUri: asOptional(asString, 'amqp://guest:guest@localhost:5672'), |
There was a problem hiding this comment.
Instead of changing this, could we change the docs? It's insecure either way, but at least the old way was more descriptive.
| } catch (error) { | ||
| logger.error('Marketing daemon error:', error) | ||
| throw error | ||
| } |
There was a problem hiding this comment.
Don't do this. The runDaemon is supposed to handle all errors. If you don't like the way it does that, fix it there, not here.
| } | ||
|
|
||
| logger.info(`Processing ${pendingTasks.length} marketing tasks`) | ||
| heartbeat(`Processing ${pendingTasks.length} tasks`) |
There was a problem hiding this comment.
The heartbeat's purpose is to rate-limit log messages. Most of these calls to heartbeat won't do anything, because it only allows one message every 10 seconds or whatever.
The correct way to use heartbeat is deep within your inner loop (so the streamDevicesByLocation loop). So, delete the heartbeat calls everywhere except the inner loop. Inside the inner loop, delete the logger.info call.
| * The marketing tasks database setup. | ||
| */ | ||
| export const couchMarketingTasksSetup: DatabaseSetup = { | ||
| name: 'db_marketing_tasks', |
There was a problem hiding this comment.
The db_ naming scheme is obsolete. The modern name would be push-marketing-tasks.
| } | ||
| } | ||
|
|
||
| await db.insert(doc) |
There was a problem hiding this comment.
You should be using db.insert(wasCouchMarketingTask(doc)), so we get type checking for these new documents.
| const result = await db.view('status', 'status', { | ||
| startkey: ['pending', ''], | ||
| endkey: ['failed', '\ufff0'], | ||
| include_docs: true | ||
| }) |
There was a problem hiding this comment.
Every 100 documents, we update the marketing task. But this update requires searching the database for the ID!
How I would fix this:
- Make the document ID is the taskId. Then we can use
db.getand avoid the search. - Don't return the raw task, but instead return a task object which has an
updatemethod. - The
updatemethod has the last-known id and rev, and can make the update directly without any DB lookups in 99% of cases. If there is a conflict error, it can refresh the _rev and try again. See the couch-lobbies implementation in edge-login-server, which also has this rapid-fire update pattern.
Also in the login server, the addKeyBox has an example of how to pick ID's that are date-based but also unique. It does this by checking for conflicts on insertion and then bumping the date by 1ms.
| // Marketing endpoints | ||
| '/marketing/count/?': pickMethod({ | ||
| GET: withMarketerApiKey(marketingCountRoute) | ||
| }), | ||
| '/marketing/send/?': pickMethod({ | ||
| POST: withMarketerApiKey(marketingSendRoute) | ||
| }), | ||
| '/marketing/send/[a-zA-Z0-9-]+/?': pickMethod({ | ||
| GET: withMarketerApiKey(marketingTaskRoute) | ||
| }), | ||
| '/marketing/sends/?': pickMethod({ | ||
| GET: withMarketerApiKey(marketingTasksListRoute) |
There was a problem hiding this comment.
In the login server, we've been putting most middleware directly around the route itself, so it's possible to see all the logic in one place. So in marketingRoutes.ts, we might see export const marketingSendRoute = withMarketerApiKey(request =>, so it's clear where the API key comes from.
| const taskId = | ||
| pathParts[pathParts.length - 1] !== '' | ||
| ? pathParts[pathParts.length - 1] | ||
| : pathParts[pathParts.length - 2] | ||
|
|
||
| if (taskId === '' || taskId === 'send') { |
There was a problem hiding this comment.
This logic seems weird. Shouldn't it just be const taskId = pathParts[pathParts.length - 2], and then if (taskId == null || taskId ==='') { return errorResonse...
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none