Open samim23 opened 10 years ago
Hey @samim23 I've come across this issue a couple of times, right now I'm trying to see if I can find a way to reproduce it. I'm also wanting to determine if this issue has been introduced in some of the later changes that have been made. Seems like @gabrielhpugliese has a similar issue: #12
ok cool. what is your recommendation for the mean time? i´m thinking about removing presence for now.
You could try downgrade to an earlier version, perhaps e59a12739685972c970f70956c4d3e78c31818ec you'll have to manually install the package to do this, just clone presence into packages.
It's hard to debug. I tried to debug localhost and coderstv.com with 50 chrome tabs, but couldn't reproduce. Do not know where to console.log either.
Hey, I've built a package just for this purpose. I do the interval on client and insert/update the presence on client too. I'm going to test it thursday with lots of different people. If it works I'll make a package.
Gabriel Pugliese CodersTV.com @coderstv
On Fri, Nov 22, 2013 at 11:43 PM, David Burles notifications@github.comwrote:
You could try downgrade to an earlier version, perhaps e59a127https://github.com/tmeasday/meteor-presence/commit/e59a12739685972c970f70956c4d3e78c31818ec you'll have to manually install the package to do this, just clone presence into packages.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29122845 .
@gabrielhpugliese very much looking forward to your patch as especially the issues with counters going up to 100 for a second seems to be getting worse and worse with scale. i´ve been researching "not using a heart" https://github.com/tmeasday/meteor-presence/issues/10/ a bit and seems like there is a more fundamental fix to be had in conjunction with that, or i´m i off track?
Well, it worked like a charm for a few minutes. I think it needs more improvement. I will open source it soon so you can help on improvements. Or probably this is a NP-hard problem :P
Gabriel Pugliese CodersTV.com @coderstv
On Thu, Nov 28, 2013 at 7:20 AM, samim notifications@github.com wrote:
@gabrielhpugliese https://github.com/gabrielhpugliese very much looking forward to your patch. i´ve been researching "not using a heart" #10https://github.com/tmeasday/meteor-presence/issues/10a bit and seems like there is a more fundamental fix to be had in conjunction with that, or i´m i off track?
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29449249 .
NP hard problem? how come a quake server browser solved this efficiently back in 1999 already? ;-) Anyhow, my issues are getting worse and worse now i´m hitting 1k daily users, even suspecting its pretty bad for the overall performance, could that be true? I´m gonna put in a day next week and will try to merge some of these libraries into a working solution, will share if i succeed.
Well, the load on my application hosted on a single-core CPU 2GHz goes to
Gabriel Pugliese CodersTV.com @coderstv
On Thu, Nov 28, 2013 at 10:34 PM, samim notifications@github.com wrote:
NP hard problem? how come a quake server browser solved this efficiently back in 1999 already? ;-) Anyhow, my issues are getting worse and worse now i´m hitting 1k daily users, even suspecting its pretty bad for the overall performance, could that be true? I´m gonna put in a day next week and will try to merge some of these libraries into a working solution, will share if i succeed.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29490516 .
Well, I just improved it. Instead of inserting things on client, defining allow rules in server, I've created a method and it's WORKINGOMG with 0.15 load on my server. Want to test ? https://github.com/gabrielhpugliese/meteor-users-counter
Gabriel Pugliese CodersTV.com @coderstv
On Fri, Nov 29, 2013 at 12:19 AM, Gabriel Pugliese < gabrielhpugliese@gmail.com> wrote:
Well, the load on my application hosted on a single-core CPU 2GHz goes to
- 95% of cpu is for main.js When I turn it off, it goes to bottom. I have no idea why. Next Meteor version will provide a package called Facts that will give for free how many subscriptions are active. Idk if it's enough for you.
Gabriel Pugliese CodersTV.com @coderstv
On Thu, Nov 28, 2013 at 10:34 PM, samim notifications@github.com wrote:
NP hard problem? how come a quake server browser solved this efficiently back in 1999 already? ;-) Anyhow, my issues are getting worse and worse now i´m hitting 1k daily users, even suspecting its pretty bad for the overall performance, could that be true? I´m gonna put in a day next week and will try to merge some of these libraries into a working solution, will share if i succeed.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29490516 .
@gabrielhpugliese you are amazing! i´m going to give this a spin right now and test it in-depth - will give you feedback asap.
Forgot to say, it works with 0.6.4.1 only - what's the meteor version you're using ?
Gabriel Pugliese CodersTV.com @coderstv
On Fri, Nov 29, 2013 at 4:06 AM, samim notifications@github.com wrote:
@gabrielhpugliese https://github.com/gabrielhpugliese you are amazing! i´m going to give this a spin right now and test it in-depth - will give you feedback asap.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29498240 .
Ah to bad, i´m stuck on meteor 0.6.6.3 for now until the new render engine reaches a stable release. Hope i will get to that soon and i´ll get to check out your fix.
Hey guys. I've just done a complete rewrite to make use of livedata. Would be awesome if you could try it out and let me know how it works for you. You can grab it from the livedata branch.
Nice David, gonna try it :) Thanks
Gabriel Pugliese CodersTV.com @coderstv
On Sun, Dec 1, 2013 at 9:12 PM, David Burles notifications@github.comwrote:
Hey guys. I've just done a complete rewrite to make use of livedata. Would be awesome if you could try it out and let me know how it works for you. You can grab it from the livedata branch.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29586213 .
@gabrielhpugliese thanks!, interested to hear how you go.
@dburles very cool stuff, thanks! I´ve been testing on my localserver and so far works like a charm! haven´t performance monitored fully yet but looking better off the bat. Might give it a shoot on the production server soon, anything to keep in mind with the transition?
@samim23 Good to hear! the main differences I can think of are that A. the _id coming from the presences collection now is tied directly to the livedata session id, so these will drop on hot code reload, where previously it would reattach the session to the existing presence _id. and B. clients not connected through a socket, (long polling) take a while to drop off when disconnecting, not sure of the exact duration, but somewhere around 1 minute. I believe though that this may have already been an issue with the previous version. Anyway it's probably useful to be aware of this limitation.
Will this work on 6.4.1 ?
Gabriel Pugliese CodersTV.com @coderstv
On Tue, Dec 3, 2013 at 10:34 AM, David Burles notifications@github.comwrote:
@samim23 https://github.com/samim23 Good to hear! the main differences I can think of are that A. the _id coming from the presences collection now is tied directly to the livedata session id, so these will drop on hot code reload, where previously it would reattach the session to the existing presence _id. and B. clients not connected through a socket, (long polling) take a while to drop off when disconnecting, not sure of the exact duration, but somewhere around 1 minute. I believe though that this may have already been an issue with the previous version. Anyway it's probably useful to be aware of this limitation.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29705895 .
@gabrielhpugliese I don't think so as it's relying on upsert which was introduced with 0.6.6
Okay :(
Gabriel Pugliese CodersTV.com @coderstv
On Tue, Dec 3, 2013 at 11:57 AM, David Burles notifications@github.comwrote:
@gabrielhpugliese https://github.com/gabrielhpugliese I don't think so as it's relying on upsert which was introduced with 0.6.6
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29710985 .
If you're feeling adventurous, it should work if you modify the upsert to the typical, check if exists, then run either an update or insert.
Or maybe update my project to 0.6.6 :P I'm postponing it forever. My smart.json have a lot of git tags of previous versions.
Gabriel Pugliese CodersTV.com @coderstv
On Tue, Dec 3, 2013 at 12:07 PM, David Burles notifications@github.comwrote:
If you're feeling adventurous, it should work if you modify the upsert to the typical, check if exists, then run either an update or insert.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29711708 .
Yeah updating the project would be a good idea :)
@dburles i´m running your new version on the production server now at https://hyperboring.com. So far works perfect and no strange user counter raises anymore. Performance is much better to, with memory consumption and CPU time both down by allot. Nice job, this is great!
You guys may want to check out https://github.com/mizzao/meteor-user-status, which I've used in many projects and seems to be pretty stable and straightforward.
I've never used it because on docs it does not mention the possibility to add custom vars to the presence doc. Is it possible ?
Gabriel Pugliese CodersTV.com @coderstv
On Tue, Dec 3, 2013 at 10:52 PM, Andrew Mao notifications@github.comwrote:
You guys may want to check out https://github.com/mizzao/meteor-user-status, which I've used in many projects and seems to be pretty stable and straightforward.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29767756 .
@gabrielhpugliese I see - I always just assumed that anyone who would want to add anything on the client side would just add it to the profile
field of a user doc, and that would be published/subscribed as normal. It didn't make sense to me to include an extra collection for this because presence information is already associated with each user (versus sessions, for example.)
For example, I need to know where an user is at the moment. I think it's perfectly reasonable to stay on the presence doc, not on profile. It's something ephemeral.
Gabriel Pugliese CodersTV.com @coderstv
On Wed, Dec 4, 2013 at 12:11 AM, Andrew Mao notifications@github.comwrote:
@gabrielhpugliese https://github.com/gabrielhpugliese I see - I always just assumed that anyone who would want to add anything on the client side would just add it to the profile field of a user doc, and that would be published/subscribed as normal. It didn't make sense to me to include an extra collection for this because presence information is already associated with each user (versus sessions, for example.)
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29771766 .
@dburles i´ve been testing your new code for an evening and works nicely, except for "ghost" users that get stuck in the system for hours even though nobody is there. happens only sporadically, only reboot seems to work as a fix.
@gabrielhpugliese the missing extra fields (and coffeescript ;-) ) was the main reason i decided against user-status. But i did not think it through it seems, as your solution described here makes perfect sense.
Yeah coffeescript +1
Gabriel Pugliese CodersTV.com @coderstv
On Wed, Dec 4, 2013 at 12:19 AM, samim notifications@github.com wrote:
@gabrielhpugliese https://github.com/gabrielhpugliese the missing extra fields (and coffeescript ;-) ) was the main reason i decided against user-status. But i did not think it through it seems, as your solution described here makes perfect sense.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29772106 .
@mizzao @gabrielhpugliese i´ve made a quick translation of the meteor-user-status / user_status.coffee from coffee into js: https://gist.github.com/samim23/7785333 not tested yet. @gabrielhpugliese solution for custom fields (querying the user profile object) makes sense so will try that, as @dburles newest fix unfortunately still produces strange results on the production server. @mizzao is it possible to track non logged in users to?
@samim23 It looks you got the inverted message from @gabrielhpugliese, who said he liked coffeescript but didn't want to keep presence data around in profile
. You might also want to update your conversion to js since I just made some changes as well :). Also, you could definitely track both logged-in and non-logged-in users with their session/socket id.
@dburles and @samim23 I just realized the ghost users may be due to an incorrect use of upsert. See the latest commits on user-status.
No, I do not like coffee :(
Gabriel Pugliese CodersTV.com @coderstv
On Wed, Dec 4, 2013 at 2:41 PM, Andrew Mao notifications@github.com wrote:
@dburles https://github.com/dburles and @samim23https://github.com/samim23the ghost users may be due to an incorrect use of upsert. See the latest commits on user-status.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29821394 .
I think we should move this conversation to another place, like meteor-talk
Gabriel Pugliese CodersTV.com @coderstv
On Wed, Dec 4, 2013 at 2:42 PM, Gabriel Pugliese <gabrielhpugliese@gmail.com
wrote:
No, I do not like coffee :(
Gabriel Pugliese CodersTV.com @coderstv
On Wed, Dec 4, 2013 at 2:41 PM, Andrew Mao notifications@github.comwrote:
@dburles https://github.com/dburles and @samim23https://github.com/samim23the ghost users may be due to an incorrect use of upsert. See the latest commits on user-status.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-29821394 .
@mizzao tracking non-logged-in users with their session/socket id is clear, question is more where to store the additional field data for those as they have no profile. Of course could open a parallel collection for that but seems messy. @dburles any clues what could cause those ghosts? I´m tempted to empty the collection serverside on a interval for now but seems hacky.
hey @samim23 I think the issue could have been caused by the upsert as @mizzao mentioned, I realised it isn't actually required. the updates are over on https://github.com/dburles/meteor-presence
@samim23 I'm curious if you see ghosts when using user-status too. Might be the way you are using it, mind giving it a try?
@dburles you definitely need the upsert; a user logging in and out and back in, etc will have the same session id and you will get duplicate key errors.
oh yeah of course, argh. too early in the morning :)
@dburles Take a look at https://github.com/meteor/meteor/issues/1552 to make sure you are using upsert correctly; if you don't include $set
then it will just ignore the key you entered, possibly causing the ghosting.
@mizzao thanks. looking into it more, i can cause ghost users to appear by refreshing the browser rapidly while logged in. also, strangely the socket close event fires twice on the same id for logged in users.
Hey so i'm pretty confident i've now corrected the ghosting issue, the one i mentioned above. So assuming that this was causing the same problems that you were seeing @samim23 hopefully it will now no longer happen! It would be great if you could try it out and let me know if there are any issues @samim23. Make sure you grab it from here: https://github.com/dburles/meteor-presence
@dburles super cool! i´ve just uploaded your new version to my production server. Not much testing yet but will give you proper feedback soon.
thanks, appreciated!
@dburles So i´ve done some testing today, two issues: Ghosts still can appear even though far less frequent. Unfortunately it stopped tracking non-logged in users completely which is bad for my usecase.
@samim23 hey yeah sorry about that, I was doing a lot of testing with logged in users. I need to figure out how to write tests for this! I've patched it up now so just grab the code again from dburles/presence. If you don't mind, when you are seeing ghost users, could you show me what they appear like in the collection?
@dburles i´ve just uploaded your most recent patch but now in my server stats the project average response time went from somewhere around 100ms to over a value of 150000ms. Evil ! As i did not change much else, i´m asking myself if this could be related to your update at all? I´ll look into making a snapshot of my collection asap and sending it over.
Hmm I'm not sure how that correlates with the changes made. Wonder if something else has gone askew. Very strange. Possible to trace down the problem?
On Sunday, 8 December 2013, samim wrote:
@dburles https://github.com/dburles i´ve just uploaded your most recent patch but now in my server stats the project average response time went from somewhere around 100ms to over 150000ms which seems to be presence related. I´m going from bad to worse with this solution :-( I´ll look into making a snapshot of my collection asap and sending it over.
— Reply to this email directly or view it on GitHubhttps://github.com/tmeasday/meteor-presence/issues/17#issuecomment-30060314 .
i´m using presence in a production app over at hyperboring.com. noticing strange spikes at time in the online users count. 3 users are online but it will show 50-100 for 3-4 seconds and then go gradually back down. Today the entire server died and when i looked at the logs, it was presence killing it. Could this be related somehow? any thoughts are super welcome!
here´s my error log http://snipt.org/Bnv7