j3k0 / ganomede-notifications

Long-pull notification service for Ganomede
0 stars 0 forks source link

Push API cleanup #17

Closed elmigranto closed 9 years ago

elmigranto commented 9 years ago

Updating README and fixing some stuff.

j3k0 commented 9 years ago

Do you think you can be done with the cleanup soon? I was hoping I can deploy it today.

One thing I realized: for now you're using the from of the notification as a value for push's app (to retrieve tokens related to the app the user is using). I realize this isn't working, if from is invitations/v1 or turngame/v1 it doesn't tell us what game/app it's related to.

I think there should be a notification.push.app field that fills this value, so that we can set it (say) to triominos/v1 or anything else.

j3k0 commented 9 years ago

More testing shows that the device isn't displaying any alert. Current code seems a bit messy in the way to generate the apn notification object, part of the apn notification object is generated using Task.converters, while another part is set from ApnSender.send. I think the whole object should be generated from the converter.

Main issues currently.

Below, extract from the code I sent you by skype, containing a valid apn.

var note = new apn.Notification();

note.expiry = Math.floor(Date.now() / 1000) + 3600; // Expires 1 hour from now.
note.badge = 1;
note.sound = "ping.aiff";
note.alert = {
    "title-loc-key": "your_turn_title",
    "loc-key": "your_turn_message"
};

note.payload = {
    from: "invitations/v1",
    gameId: 12345,
    data: {
        a: 1,
        b: "string-2"
    }
};
elmigranto commented 9 years ago

I was a bit confused about what is what (.alert, .payload, other fields), but after reading this I think I got it right. I still not sure about couple of things:

  1. formatting ganomede .push field to apn alert you described in previous pull, because having both, title and title-loc-key seems excessive (as well as body and loc-key);
  2. what ganomede .push field could actually be. For now it supports String (.alert will be that string) and Objects (.alert will be converted to *-loc-key and *-loc-args). Otherwise it takes default string from config as an .alert and log.warns about it.

One thing I realized: for now you're using the from of the notification as a value for push's app

I think there should be a notification.push.app field that fills this value

Oh, that's true, oops. from is service that sent ganomede notification and push.app is app that should receive it, will fix.

Okay, it sounds like the right time to document notification.push

elmigranto commented 9 years ago

Do you think you can be done with the cleanup soon? I was hoping I can deploy it today.

Lets decide on .push format, I'll add error handling to ApnSender, and we can probably do it today.

elmigranto commented 9 years ago

Okay, I spent some time reading apn docs and sources and it looks like our current 'fire-and-forget' sender should do just fine and can be deployed. Though for stuff like removing no longer valid tokens, we will need to make some adjustments, but I guess that's not required for now.

j3k0 commented 9 years ago

Great, we just needs the app thing yet.

Having both *loc-key and body / title seems unnecessary, let's just wipe that out.

One thing worries me: it seems like your code is doing this:

  1. rpop
  2. has a task?
  3. no. exit
  4. yes. send
  5. wait for complete
  6. back to 1

Depending on ping and apple servers' response time, send to complete may take some time... say 1 second. Thus we can't send more than 1 message per second.

Do you think it'll be possible to do:

  1. rpop
  2. has a task?
  3. yes. send. back to 1
  4. no. wait for all complete (if anything was sent)
  5. exit

Would that work with vasync.parallel?

elmigranto commented 9 years ago

Having both *loc-key and body / title seems unnecessary, let's just wipe that out.

Which, title and body or localization ones?

Regarding algorithm, you are right, there's async queue/barrier that would be most appropriate, will look into that.

j3k0 commented 9 years ago

Which, title and body or localization ones?

keep only localized ones. (loc-key and friends)

elmigranto commented 9 years ago

Everything critical should be in order. I'll work on algorithm change tomorrow if you don't mind.

elmigranto commented 9 years ago

I redid SenderCli, it no longer waits for notification to be sent before rpoping next one. Instead, it uses node streams to control concurrency. There are some things to improve, but this should be ready.

I didn't went for exactly what you suggested (rpop until empty adding popped values to the queue), so we don't buffer the whole redis list if sending is stuck, and don't lose too many messages in case process dies.

elmigranto commented 9 years ago

There's stuff I'd like to improve, but nothing big, just dig a bit deeper into apn internals, because I'm not quite sure it does what it says (in terms of parallel connections), maybe change how we use it, and possibly see about Feedback service.

Those aren't crucial and I think we can go production and see how it performs. Let me know what you think.

j3k0 commented 9 years ago

Hey @elmigranto

Can't get the push notifications to be sent...

Edited invitations code to add push object: https://github.com/j3k0/ganomede-invitations/blob/885ddcae9756df6b384dd4af9d03de41c7037bb2/src/invitations-api.coffee#L185-L195

Below is the server trace, showing 3 successive attempts:

Jul 18 12:33:34 staging-proxy invitations-1-0-8-staging-1.2596f5c2:  {"name":"invitations","hostname":"invitations-1-0-8-staging-1","pid":17,"level":30,"msg":"sending notification { url: 'http://directstaging.ggs.ovh:80/notifications/v1/messages',\n  notification: \n   { from: 'invitations/v1',\n     to: 'j1',\n     type: 'invitation-deleted',\n     data: { reason: 'cancel', invitation: [Object] },\n     secret: 'xxx' } }","time":"2015-07-18T09:33:34.289Z","v":0} 
Jul 18 12:33:47 staging-proxy turngame-1-1-3-staging-2-1.8ee4f600:  {"name":"turngame","hostname":"turngame-1-1-3-staging-2-1","pid":113,"rulesClient":"/triominos/v1","level":30,"url":"/triominos/v1/games","msg":"post /games","time":"2015-07-18T09:33:47.413Z","v":0} 
Jul 18 12:33:47 staging-proxy invitations-1-0-8-staging-1.2596f5c2:  {"name":"invitations","hostname":"invitations-1-0-8-staging-1","pid":17,"level":30,"msg":"sending notification { url: 'http://directstaging.ggs.ovh:80/notifications/v1/messages',\n  notification: \n   { from: 'invitations/v1',\n     to: 'j1',\n     type: 'invitation-created',\n     data: { invitation: [Object] },\n     push: \n      { app: 'triominos/v1',\n        type: 'invitation_received',\n        title: [Object],\n        message: [Object] },\n     secret: 'xxx' } }","time":"2015-07-18T09:33:47.675Z","v":0} 
Jul 18 12:33:49 staging-compute push-worker-1-3-1-1.ba0b2fa7:  read 24 
Jul 18 12:33:49 staging-compute push-worker-1-3-1-1.ba0b2fa7:  written 24 
Jul 18 12:33:49 staging-compute push-worker-1-3-1-1.ba0b2fa7:  state {"nMax":4,"nWaiting":1,"readyFunctions":[]} 
Jul 18 12:35:30 staging-proxy invitations-1-0-8-staging-1.2596f5c2:  {"name":"invitations","hostname":"invitations-1-0-8-staging-1","pid":17,"level":30,"msg":"sending notification { url: 'http://directstaging.ggs.ovh:80/notifications/v1/messages',\n  notification: \n   { from: 'invitations/v1',\n     to: 'j1',\n     type: 'invitation-deleted',\n     data: { reason: 'cancel', invitation: [Object] },\n     secret: 'xxx' } }","time":"2015-07-18T09:35:30.287Z","v":0} 
Jul 18 12:35:47 staging-proxy turngame-1-1-3-staging-2-1.8ee4f600:  {"name":"turngame","hostname":"turngame-1-1-3-staging-2-1","pid":113,"rulesClient":"/triominos/v1","level":30,"url":"/triominos/v1/games","msg":"post /games","time":"2015-07-18T09:35:47.201Z","v":0} 
Jul 18 12:35:47 staging-proxy invitations-1-0-8-staging-1.2596f5c2:  {"name":"invitations","hostname":"invitations-1-0-8-staging-1","pid":17,"level":30,"msg":"sending notification { url: 'http://directstaging.ggs.ovh:80/notifications/v1/messages',\n  notification: \n   { from: 'invitations/v1',\n     to: 'j1',\n     type: 'invitation-created',\n     data: { invitation: [Object] },\n     push: \n      { app: 'triominos/v1',\n        type: 'invitation_received',\n        title: [Object],\n        message: [Object] },\n     secret: 'xxx' } }","time":"2015-07-18T09:35:47.526Z","v":0} 
Jul 18 12:35:47 staging-compute push-worker-1-3-1-1.ba0b2fa7:  read 26 
Jul 18 12:35:47 staging-compute push-worker-1-3-1-1.ba0b2fa7:  written 26 
Jul 18 12:35:47 staging-compute push-worker-1-3-1-1.ba0b2fa7:  state {"nMax":4,"nWaiting":1,"readyFunctions":[]} 
Jul 18 12:36:46 staging-proxy invitations-1-0-8-staging-1.2596f5c2:  {"name":"invitations","hostname":"invitations-1-0-8-staging-1","pid":17,"level":30,"msg":"sending notification { url: 'http://directstaging.ggs.ovh:80/notifications/v1/messages',\n  notification: \n   { from: 'invitations/v1',\n     to: 'j1',\n     type: 'invitation-deleted',\n     data: { reason: 'cancel', invitation: [Object] },\n     secret: 'xxx' } }","time":"2015-07-18T09:36:46.817Z","v":0} 
Jul 18 12:36:51 staging-proxy turngame-1-1-3-staging-2-1.8ee4f600:  {"name":"turngame","hostname":"turngame-1-1-3-staging-2-1","pid":113,"rulesClient":"/triominos/v1","level":30,"url":"/triominos/v1/games","msg":"post /games","time":"2015-07-18T09:36:51.811Z","v":0} 
Jul 18 12:36:52 staging-proxy invitations-1-0-8-staging-1.2596f5c2:  {"name":"invitations","hostname":"invitations-1-0-8-staging-1","pid":17,"level":30,"msg":"sending notification { url: 'http://directstaging.ggs.ovh:80/notifications/v1/messages',\n  notification: \n   { from: 'invitations/v1',\n     to: 'j1',\n     type: 'invitation-created',\n     data: { invitation: [Object] },\n     push: \n      { app: 'triominos/v1',\n        type: 'invitation_received',\n        title: [Object],\n        message: [Object] },\n     secret: 'xxx' } }","time":"2015-07-18T09:36:52.132Z","v":0} 
Jul 18 12:36:52 staging-compute push-worker-1-3-1-1.ba0b2fa7:  read 28 
Jul 18 12:36:52 staging-compute push-worker-1-3-1-1.ba0b2fa7:  written 28 
Jul 18 12:36:52 staging-compute push-worker-1-3-1-1.ba0b2fa7:  state {"nMax":4,"nWaiting":1,"readyFunctions":[]} 

I can't really spot an error... but the push doesn't get through.

I'll try to debug that, first I made sure API_SECRET isn't sent together with the push notification (that would be a serious security flaw).

elmigranto commented 9 years ago

Created PR https://github.com/j3k0/ganomede-notifications/pull/18.