Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#5328 closed enhancement (fixed)

Use of the WordPress HeartBeat API to check for newest activities and to let user load them

Reported by: imath's profile imath Owned by: imath's profile imath
Milestone: 2.0 Priority: normal
Severity: normal Version: 1.9.1
Component: Activity Keywords: has-patch 2nd-opinion needs-testing
Cc:

Description

Hi,

I was playing with the Activity Administration screen, when i thought : in this area the WordPress HeartBeat API should be running.. So i've played with it a bit. Then, i thought it could be interesting to play with this new API in the front end.

What i've build surely needs to be optimized, it is very tricky sometimes, and there may still be things behave the wrong way. But before trying to make it better, i'd like to have your opinion on this feature.

This initial patch is checking every 15 seconds the timestamp of the last activity recorded against the one of the first row of the activity list in Activity directory or in the groups home page. If the last 'date_recorded' field value (converted in timestamp) in the activity table is greater than the one of the first activity displayed, then it displays a "Load Newest" link above the activity listing to inform there are 1 or more new activities posted. Clicking on this link "prepends" the new activities into the activity listing.

I've added a screenshot, and the patch to this ticket.

Attachments (6)

5328.diff (16.2 KB) - added by imath 10 years ago.
activity-heartbeat.png (45.9 KB) - added by imath 10 years ago.
5328.02.diff (26.9 KB) - added by imath 10 years ago.
5328.03.diff (22.4 KB) - added by imath 10 years ago.
5328.04.diff (22.9 KB) - added by imath 10 years ago.
5328.05.diff (24.6 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (34)

@imath
10 years ago

#1 follow-up: @r-a-y
10 years ago

Sweet! This will definitely be a great feature for 2.0.

Some quick thoughts:

  • Instead of introducing a new static method (BP_Activity_Activity::get_lastest_recorded()), would it be better to improve the existing BP_Activity_Activity::get_last_updated() static method instead?
  • Instead of checking against the date_recorded column, would it be better to use the id column?
  • I'm surprised we don't have a bp_is_activity_directory() function!

We'll also need an option to disable the Heartbeat feature for sites that want to turn it off. Other than that, I think it's a very good start!

#2 in reply to: ↑ 1 @imath
10 years ago

Replying to r-a-y:

Sweet! This will definitely be a great feature for 2.0.

Thanks a lot r-a-y :)

  • Instead of introducing a new static method (BP_Activity_Activity::get_lastest_recorded())

At the beginning that's what i've done, then i thought for a first draft i should do my stuff in a new one. But i agree.

  • Instead of checking against the date_recorded column, would it be better to use the id column?

it would be simplier than manipulating a timestamp, and would avoid some extra template tags for sure. The reason i've used date_recorded must be linked to the fact that i've started by using BP_Activity_Activity::get_last_updated() and after i thought that it could be interesting for a plugin to eventually get activities older than a certain date using the new filter.

  • I'm surprised we don't have a bp_is_activity_directory() function!

Should i create a separate ticket, it would be interesting to use it. Also, i need to explore a bit bp_is_group_activity because i think if group home page displays the activities, this function might return true instead of false.

We'll also need an option to disable the Heartbeat feature for sites that want to turn it off.

I agree at 100% :)

#3 follow-ups: @boonebgorges
10 years ago

  • Milestone changed from Awaiting Review to 2.0

Wow, this is really great.

I agree with r-a-y's comments, especially that we should offer a way to turn this off. I wonder what the performance implications would be if a site had many concurrent visitors.

#4 in reply to: ↑ 3 @henry.wright
10 years ago

imath,

Very neat use of the WP Heartbeat API!

Replying to boonebgorges:

I wonder what the performance implications would be if a site had many concurrent visitors.

Making the feature optional (enable or disable options) like you suggested could help people on shared hosts if they notice a drag. Alternatively you could check every 30 seconds instead of every 15?

#5 @mpa4hu
10 years ago

what about long polling instead of short polling? I use something like this and now I'm thinking about remaking it to long polling.

This can be option to choose between short and long polling

#6 in reply to: ↑ 3 ; follow-up: @imath
10 years ago

  • Owner set to imath
  • Status changed from new to assigned

Replying to boonebgorges:

Wow, this is really great.

Thanks Boone, yes using latest_id should really ease the process and i think to turn it off a new setting in the Activity section would be nice

Replying to henry.wright:

Wow, this is really great. (...) Alternatively you could check every 30 seconds instead of every 15?

Thanks Henry, Henry & @mpa4hu : to change the pulse i think the best is to introduce a filter in the function i use to change WordPress's one. This way we can still check the filtered value is in the WordPress 'allowed' range of values : 15 to 60 seconds, and we keep things simple for administrators in the BuddyPress settings area : turn it on / turn it off

I think, we also need to consider 'activity posting' when the link 'Load newest' is displayed, by taking in account active tab (potential tabs : All Members, My Friends, My Groups, My Favorites, Mentions) & selected filter on The Activity directory page.

The way last date_recorded (which will soon be last id recorded) is get is taking in account scope & filter. For instance if on the "My Groups" tab, the link "load newest" won't be displayed even if there's a newest activity if it's not a "group component" one (i think i need to improve what i've done by also checking the user_id).

The way an activity is posted then displayed into the stream doesn't care about the active tab or active filter. For instance : if on the 'Mentions' tab, when posting a new activity it is displayed before the mentions. If you click again on the 'Mentions' tab it "disappears" ;)
And this is fine as it shows the user his activity has been posted.

Now, in the state the patch is : if the 'Load Newest' link is displayed and an activity is posted, the activity will be displayed before the 'Load Newest' link and when clicking on it older activities than the 'just posted' activity will be displayed before the 'just posted' one. As a result the chronology of the stream will temporarily be messed up till the complete reload of the page.

So to make the patch evolve the best way, i think we need to "rethink" about the way a just posted activity is displayed.

1- if the link "load newest" is displayed then in bp_legacy_theme_post_update the argument of bp_has_activities shouldn't be "include" but something like 'last_id' displayed on the stream so that the chronology of the stream is respected.
2- an extra class might be temporarily added to the just_posted activity to show the user, his activity has been posted as more than one will be displayed.
3- which would actually be the first step : i think we need to be sure the filter & scope are : activity_update & All Members. The easiest way to do that is to trigger some actions on the textarea focus if it's not the case.

Sorry for that quite long comment ;) But i needed to write these thoughts to have your opinion about them.

#7 in reply to: ↑ 6 @henry.wright
10 years ago

Replying to imath:

to change the pulse i think the best is to introduce a filter in the function i use to change WordPress's one.

Good idea!

#8 @modemlooper
10 years ago

This should not be in core but a plugin would be great. Not all sites will want that javascript running constantly. Or at least not have it unless a theme specifies it. These types of features are deciding the UX/UI for a site and that's no good.

From now on can we have new features done as feature plugins? If a feature doesn't make core than at least it's ready as a plugin.

Last edited 10 years ago by modemlooper (previous) (diff)

@imath
10 years ago

#9 @imath
10 years ago

  • Keywords needs-testing added

In 5328.02.diff, i've taken in account the above comments.

  1. The patch now improves BP_Activity_Activity::get_last_updated() instead of creating a new static method in BP_Activity_Activity
  2. HeartBeat in activity streams is not a default feature. Administrator will need to activate the option from the Activity settings section. It can be turned off/on at any time by enabling/disabling this option.
  3. the filter bp_activity_heartbeat_pulse can be used to change the pulse of activity stream heartbeat
  4. i've edited the 'post_update' process to not only display the created activity once posted but all the new activities
  5. to avoid any trouble with scope/filtering.. on #whats-new blur, i trigger some actions to be sure the scope is All and filter is everything

Seems to work fine, but surely needs some more testing ;)

#10 @DJPaul
10 years ago

PHP

  • Please can the code tidy-up bits be removed from this patch (perhaps just commit that tidy-up now)? Specifically the if ( ! empty( $pid_sql ) ) spacing changes.
  • The introduction of bp_is_activity_directory() as a new helper function should be in its own ticket which you could commit straightaway. It might also make sense to introduce similar functions for all core components that have a directory at the same time to provide a more consistent API, though those shouldn't be a reason to hold up your work on this feature.
  • The changes to get_last_updated() should definitely be committed separately from the introduction of any heartbeat functionality. Since this introduces new direct SQL calls, this could also use unit tests to help prevent us accidentally breaking this in the future.
    • Have you EXPLAIN'd those queries to check we are hitting indexes, etc. ?
    • Do we not have a core function that already builds a query that does this for us, or is it more optimal to do this directly?
    • We should definitely object cache anything that we're proposing to hit every X seconds to avoid a bunch of database queries.
  • Changing the heartbeat interval with heartbeat_settings will affect any heartbeat request, not just ones from BuddyPress. There are very few plugins that do front-end heartbeats, but it's worth keeping in mind.
    • I think we load bp-activity-filters.php inside wp-admin, and I think our function here will affect the heartbeat in the various wp-admin screens.
  • A level of indentation can be removed from bp_activity_heartbeat_last_recorded() if we bail out at the top of the function, if the request doesn't contain any BP data.

JS

  • We should namespace the JS event handlers in case other plugins choose to remove them via JS in the future. It's pretty easy to do: change e.g. on( 'heartbeat-send', ... ) to on( 'heartbeat-send.buddypress', ... ).
  • In the heartbeat-tick JS handler, we don't do anything if the request doesn't contain data['last_id_recorded']. I think we should prefix that key in case another plugin implements heartbeat using that same ID. e.g. something like bp_activity_last_id_recorded.
  • Some minor indentation issues.
  • If the browser/tab is in the background or out of focus, we should not send any heartbeat requests to save data usage. I am not sure if this kind of enhancement has been added to WP Core since I last looked at the heartbeat code, but it's not too hard to implement manually.

IMO if we are introducing a new feature, it should be on by default if its component is on. I implemented front-end heartbeat in another plugin and user feedback was that I needed to speed up the heartbeats -- people weren't on pages for longer than 15 seconds (or whatever the default ping period is) so never saw the effects, and no-one yet has complained that their crappy webhost can't handle extra requests. Something to keep in mind.

#11 @boonebgorges
10 years ago

I was going to come here and make a couple points (unit tests, on-by-default, cleanups in a separate commit), but DJPaul beat me to it. +1 to everything he says here.

#12 @imath
10 years ago

Thanks DJPaul and boonebgorges for your feedbacks.

PHP :

  • point 1 & 3 : #5409
  • point 2 : #5408
  • point 4, i'm not sure the heartbeat settings in wp-admin would be modified as the filter only changes it if bp_activity_do_heartbeat() ( activity directory or group activity or group home page if no custom front)

I'll wait for #5408 & #5409 to be fixed to carry on with this ticket.

#13 @boonebgorges
10 years ago

Thanks for splitting it up a bit, imath.

i'm not sure the heartbeat settings in wp-admin would be modified as the filter only changes it if bp_activity_do_heartbeat() ( activity directory or group activity or group home page if no custom front)

On reflection, I think it's probably a good idea for us *not* to mess with global heartbeat settings like this, whether on the front-end or back-end. Over time, more and more plugins will be using it, and I don't want BuddyPress to cause headaches because it's being a bully :) If we're concerned about the overhead that the BP-specific queries will cause during each heartbeat, we can set an internal timer, and bail out if we're within the BP-specific interval. For example, if someone changes their heartbeat interval globally to 5 seconds, we could bp_update_option( 'bp_activity_last_heartbeat', time() ) and then not run our own queries until 10 or 15 or 30 seconds have passed, or whatever. (I'm not really recommending this, FWIW - by the time the heartbeat has happened, WP has already mostly loaded, so the overhead damage has already been done - our own query is very tiny by comparison.)

@imath
10 years ago

#14 @imath
10 years ago

In 5328.03.diff :
a) Last comment by boonebgorges on #5409 made me think of a different way to load the newest activities. I do not need to modify the BP_Activity_Activity::get_last_updated() method anymore as i'm using an activity loop to directly build the list of activities, then display it from buddypress.js. I thought that it would avoid the query to check for the latest id, doing this way.
b) About heartbeat_settings, i'm suggesting an alternative way to set the heartbeat pulse : from buddypress.js.
c) the heartbeat-send is namespaced to heartbeat-send.buddypress
d) i've prefixed all keys in JS
e) about Heatbeat pulse, i've set it to 15s, but we could set it to 5s ("fast") because as DJPaul said "people weren't on pages for longer than 15 seconds". Moreover in heartbeat.js, there's an interesting comment :

* When setting to 'fast' or 5, by default interval is 5 sec. for the next 30 ticks (for 2 min and 30 sec).
* In this case the number of 'ticks' can be passed as second argument.
* If the window doesn't have focus, the interval slows down to 2 min.

f) Activity HeartBeat option is now on by default

#15 follow-up: @boonebgorges
10 years ago

Hey imath - Looking better. I like the new technique of using the activity loop inside of an output buffer. A couple smallish comments:

  • You're caching both the latest activity *id* and the *markup*. When you cache the markup, it makes invalidation more complicated: you should be deleting this cache when the 'activity' slug is changed, when the activity user updates his display name, etc etc. For simplicity's sake, I'd recommend caching the latest activity ID only. The additional overhead required to fetch the activity data is pretty minor, and it'll only happen when there's actual new activity to fetch.
  • The new $filter['id'] param is confusingly named, IMO. How about 'offset' instead of 'id'?
  • Can we add '_bp_enable_heartbeat_refresh' to bp_get_default_options()? This'll help us to avoid cache misses when the setting has not been set; see #5385
  • Given that the heartbeat interval is shared by all plugins, I think we shouldn't mess with it. We should, however, add a page to the codex about the feature, and explain how the interval can be made longer or shorter.

@imath
10 years ago

#16 in reply to: ↑ 15 @imath
10 years ago

Replying to boonebgorges:

Hey imath - Looking better. I like the new technique of using the activity loop inside of an output buffer.

Thanks boonebgorges :)

About caching : i agree with your point, but then, i think we don't need to use caching. Because the latest id is not a result of a query but of a parsing of the first li#id done in buddypress.js. So in 5328.04.diff i've removed all caching stuff.
I've changed new $filter['id'] in favor of $filter['offset']
I've added '_bp_enable_heartbeat_refresh' to bp_get_default_options()

About HeartBeat pulse :
I understand your prudence (< i don't know if it's the same meaning in english than in french) about not messing in this area.
On the other hand, if no interval is set, then the refresh will never happen, so users will be a bit surprised to see that the option is on by default and nothing happens.

in 5328.04.diff, i suggest to deal with this point :

1/ Does any plugin is adding an interval ?

  • if so use it
  • if no, use 15s

2/ Is the bp_activity_pulse filtered ?

  • If so use the filtered value even if plugin added an interval.
  • if no use the one set by the plugin or by BuddyPress

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.


10 years ago

#18 follow-up: @boonebgorges
10 years ago

So in 5328.04.diff i've removed all caching stuff.

Yeah, this seems right to me.

I understand your prudence (< i don't know if it's the same meaning in english than in french)

Oui, c'est exact :)

I think this is just about ready to go. 5328.05.diff contains a few changes:

  • I changed the 'offset' param so that passing 'offset=35' would get items *greater than or equal to* 35, instead of greater than. This seems more like how "offset" should work, and it seems like it's how WP_Query handles its own offset parameter. I made the necessary +1 change in the code so that the heartbeat would continue to work.
  • Unit test for 'offset'
  • Some documentation and codestyling cleanup
  • I like your new logic for setting the pulse interval, but I rewrote the documentation and flipped around some variable names inside of bp_activity_heartbeat_strings() so that it made a bit more sense. (Some of the logic you were using before was kinda opaque - it took me like 20 minutes to understand the function :) )
  • I changed the wording of the admin setting (bp_admin_setting_callback_heartbeat()). For one thing, "15 seconds" was not always going to be literally true. For another, the word "refresh" is a little misleading - the page doesn't really refresh, it just checks to see if there's new items available. I changed it to "Query automatically for new items when viewing the activity stream", but if you have ideas for how that could be improved, please feel free.
  • Messed with your JS a little bit so that one of the jQuery objects would get cached in a variable

#19 in reply to: ↑ 18 @imath
10 years ago

Replying to boonebgorges:

I think this is just about ready to go.

Great :)

  • I like your new logic for setting the pulse interval, but I rewrote the documentation and flipped around some variable names inside of bp_activity_heartbeat_strings() so that it made a bit more sense. (Some of the logic you were using before was kinda opaque - it took me like 20 minutes to understand the function :) )

Oops sorry for this :( my english is terrible sometimes!!

  • I changed the wording of the admin setting (bp_admin_setting_callback_heartbeat()). For one thing, "15 seconds" was not always going to be literally true. For another, the word "refresh" is a little misleading - the page doesn't really refresh, it just checks to see if there's new items available. I changed it to "Query automatically for new items when viewing the activity stream", but if you have ideas for how that could be improved, please feel free.

Yes, you're right, "Query" seems fine to me or alternatively we could use "Automatically check.."

I've run some tests and it works really fine.
Thanks a lot for looking at it and improving it the way you did :)

#20 @boonebgorges
10 years ago

In 7951:

Add 'offset' parameter to bp_has_activities() function stack

This param allows plugins and themes to filter results to only those activity
items whose IDs are greater than the 'offset' parameter. See eg the "load
new items" integration with the Heartbeat API.

See #5328

Props imath, boonebgorges

#21 @boonebgorges
10 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 7952:

Automatically check for newly created items when viewing the Activity Stream.

Leveraging WP's Heartbeat API, this new functionality performs periodic checks
to see whether new activity items matching the current filter have been posted
since the page was originally loaded. If new items are found, a Load Newest
link is inserted into the top of the stream, which will insert the new items.

A filter is included, bp_activity_heartbeat_pulse, that allows site owners to
set a specific interval for these checks. The default value is 15 seconds, or
whatever your global Heartbeat interval is.

To disable the feature, there is a new setting "Automatically check for new
activity items when viewing the activity stream".

Fixes #5328

Props imath, boonebgorges

#22 @DJPaul
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

On https://buddypress.trac.wordpress.org/browser/trunk/bp-templates/bp-legacy/buddypress-functions.php?rev=7952#L687 please can we not have an assignment operator used in an if statement like this? It's pretty hard to follow, and there's a warning about this in the WP code standards.

#23 @boonebgorges
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 7953:

Clarify syntax related to r7952.

Fixes #5328

#24 @aaclayton
10 years ago

I'd like to chime in on this issue as a potential detractor. I think the spirit behind this addition to BuddyPress is great, but I really don't think the Heartbeat API is yet to a state where it should be enabled on the frontend. Heartbeat (by default) calls admin-ajax every 15 seconds for every active user. On top of normal page requests and normal one-time ajax calls, this can be a massive blow for large sites.

Here's a short blog post I saw related to this point: http://www.markomedia.com.au/admin-ajax-php-high-cpu-problem-solved/
And some more evidence: http://wordpress.org/support/topic/admin-ajaxphp-being-called-from-admin-pages-causing-db-connection-issues

On my site (for example) I often have upward of 200 concurrent logged-in users, I don't want heartbeat coming anywhere near my frontend. In fact, admin-ajax is already pretty inefficient. So much so, that for an ajax heavy theme (which every BP theme would be if heartbeat is integrated) you are much better off routing theme-side ajax calls through a separate custom frontend AJAX handler which is far less resource intensive.

The heartbeat API is great for the WordPress backend, but let's face it, WP is not by nature a membership site platform. Heartbeat was designed to allow a team of authors to avoid stepping on each other's toes.

Ultimately, an option to disable this would be sufficient, but I can't really be excited about this even as an optional feature.

Last edited 10 years ago by aaclayton (previous) (diff)

#25 @boonebgorges
10 years ago

aaclayton - Thanks very much for the feedback. I agree that any AJAX-heavy BP features are problematic for large sites, and I definitely agree that the admin-ajax.php implementation leaves a lot to be desired. The core team has plans to build a custom AJAX endpoint in a future release for just the reasons you suggest.

The nature of this particular feature is such that it likely won't add a huge amount of overhead for most sites, even those that have many concurrents. It only kicks in if a single user is idling on an activity stream page for 15 seconds or more. I have a feeling that this will not happen often for the vast majority of BuddyPress sites.

That said, it's possible that your site will be one of the outliers (that's a good problem to have :) ). For people like you, there's an admin toggle to disable the feature. And, as with many other BP features, we'll continue to work on making this one scale better in future releases.

Thanks as always for your thoughtful comments! It's extremely valuable to get feedback from people running busy sites.

#26 @DJPaul
10 years ago

We should bump the minimum supported WP version to 3.6, as that's when the heartbeat API was introduced. We used to seem to be more rigorous about bumping the minimum version to whatever the latest WP version was at time of release, as there's no reason to encourage people to stay on out-of-date WordPresses.

#27 @boonebgorges
10 years ago

We used to seem to be more rigorous about bumping the minimum version to whatever the latest WP version was at time of release, as there's no reason to encourage people to stay on out-of-date WordPresses.

IIRC, this bumping was arbitrary. I strongly oppose bumping our minimum requirements just because we think people "ought to" be running a certain version of WP. We should encourage updates, but not by the manipulation of something that ought to be driven by technical considerations.

That said, a number of issues have come up, this being the most significant, that suggest that we ought to drop support for WP 3.5.x. I'm OK with that.

#28 @boonebgorges
10 years ago

In 7991:

Bump minimum WordPress requirement to 3.6

BuddyPress 2.0 introduces a number of features that require WP 3.6, including
some use of wp_unslash() and integration with the Heartbeat API.

See #5328, #5431 et al

Props DJPaul

Note: See TracTickets for help on using tickets.