WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 21 months ago

#41217 closed enhancement (fixed)

WP_Community_Events Disable Event Logs

Reported by: Howdy_McGee Owned by: iandunn
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

Every time a user logs into WordPress, WP_Community_Events triggers maybe_log_events_response() writing to the error log that it received data, even whenever it's valid data:

[14-Jun-2017 11:16:07 UTC] WP_Community_Events::maybe_log_events_response: Valid response received. Details: {$JSON_DATA}

Where $JSON_DATA is the returned information. It's valid, there's no real issue I would just like the ability to turn off these logs or disable it from writing whenever the response is successful. It's just additional unnecessary noise in the debug log.

Attachments (6)

41217.diff (552 bytes) - added by voldemortensen 2 years ago.
41217.2.diff (811 bytes) - added by voldemortensen 2 years ago.
41217.3.diff (1.2 KB) - added by iandunn 22 months ago.
Minor style tweaks, switch to a constant
41217.4.diff (2.4 KB) - added by iandunn 22 months ago.
Remove the logs entirely, see comment:11
log-community-events-requests.php (1.4 KB) - added by iandunn 22 months ago.
Example mu-plugin to log API requests
41217.5.diff (2.3 KB) - added by iandunn 22 months ago.
Deprecate the method instead of removing it

Download all attachments as: .zip

Change History (20)

#1 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 4.9

@voldemortensen
2 years ago

#2 @voldemortensen
2 years ago

  • Keywords has-patch added

#3 @iandunn
2 years ago

I wonder if we should actually just turn the logs off by default.

We originally added them because the info is very helpful in troubleshooting reports of problems (like incorrect geolocation/geocoding results), but we haven't been getting many, so it's probably not worth cluttering logs for the 99%.

In that case, it'd probably be best to remove the WP_DEBUG_LOG condition and just use the filter, so that, when we do ask people for the logs, it's only 1 step to enable them, rather than 2.

#4 follow-up: @voldemortensen
2 years ago

@iandunn good point. 41217.2.diff simplifies that conditional.

#5 @SergeyBiryukov
2 years ago

The filter needs to be documented as per the PHP Documentation Standards.

#6 follow-up: @charlestonsw
23 months ago

My 2-cents -

You're proposing adding another filter which is a lot of extra overhead. You're adding data to the memory stack and forcing WP Core to pull through the filter list and run callbacks.

IMO it would be a better solution overall to force the Community Events developers to add a define to wp-config.php if they DO want to execute this logging. This seems like a far simpler solution and one that is specific to the class at hand versus hijacking the default DEBUG_LOG behavior for a corner case.

		if ( ! WP_COMMUNITY_EVENTS_LOG ) {
			return;
		}
Last edited 22 months ago by SergeyBiryukov (previous) (diff)

#7 in reply to: ↑ 6 ; follow-ups: @iandunn
23 months ago

Replying to charlestonsw:

You're proposing adding another filter which is a lot of extra overhead. You're adding data to the memory stack and forcing WP Core to pull through the filter list and run callbacks.

I think that's a good point. Even a tiny thing like this adds up when combined with all the other filters.

if ( ! WP_COMMUNITY_EVENTS_LOG ) {

I think it'd need to be if ( ! defined( 'WP_COMMUNITY_EVENTS_LOG' ) || ! WP_COMMUNITY_EVENTS_LOG ), since PHP evaluates undefined constants as a string with the name as the value (i.e., 'WP_COMMUNITY_EVENTS_LOG'), which then evaluates to true.

A name likeWP_DEBUG_LOG_EVENTS might be a bit better, since it'd be consistent with the related constants.

#8 in reply to: ↑ 4 @iandunn
23 months ago

  • Component changed from Widgets to Administration
  • Focuses administration removed
  • Owner set to iandunn
  • Status changed from new to accepted

Replying to voldemortensen:

41217.2.diff simplifies that conditional.

This is minor and maybe just personal preference, but I think the early return is a bit more readable than putting the logic inside the if.

It avoids an unnecessary layer of indentation, and separates the function's pre-condition from the function's core logic (both visually and mentally), so that they're smaller and separate chunks. I think that makes it easier and quicker to mentally process them when skimming the function, at least for me.

It also keeps the diff much smaller and targeted to the exact change that's desired.

#9 in reply to: ↑ 7 @iandunn
23 months ago

Replying to iandunn:

I think it'd need to be if ( ! defined( 'WP_COMMUNITY_EVENTS_LOG' ) || ! WP_COMMUNITY_EVENTS_LOG )

Actually, it'd probably be better to just explicitly define a default false value, so that the constant gets documented in wp_initial_constants().

@iandunn
22 months ago

Minor style tweaks, switch to a constant

#10 @iandunn
22 months ago

Based on some feedback in Slack, it sounds like filters are Core's preferred approach. There's a tiny bit of overhead, but they're more flexible. The overhead is less than it used to be, though, thanks to r38571.

#11 @dd32
22 months ago

IMHO: The logging features/functionality of the widget should be pulled entirely, as it's not useful to the majority of users (and as far as I can tell, it's usefulness is decreasing day-by-day, as conditions requiring it should be decreasing).

If debugging data is required, it feels like this would be better suited to a plugin which can expose the data in the WordPress interface in a useful manner, reachable by the non-technical users and those without log access on their host (quite a lot of people have no idea how to access those).
If hooks are needed to facilitate a debugging plugin, add them.

@iandunn
22 months ago

Remove the logs entirely, see comment:11

@iandunn
22 months ago

Example mu-plugin to log API requests

#12 @iandunn
22 months ago

Actually, now that you mention that, I agree that that'd be even better, thanks :)

A plugin can get the needed data via http_api_debug. An example of that is log-community-events-requests.php.

41217.4.diff removes the logging entirely.

@iandunn
22 months ago

Deprecate the method instead of removing it

#13 @iandunn
22 months ago

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

In 41316:

Dashboard: Discontinue nonessential logging of Events API requests.

These log entries are only useful when troubleshooting unexpected results from the API, which is not common. The vast majority of users are better served by not having their logs cluttered with noise.

For the rare situations where troubleshooting is necessary, it can be achieved by a plugin (see #41217 for an example).

Props Howdy_McGee, dd32.
Fixes #41217.

#14 in reply to: ↑ 7 @charlestonsw
21 months ago

if ( ! WP_COMMUNITY_EVENTS_LOG ) {

I think it'd need to be if ( ! defined( 'WP_COMMUNITY_EVENTS_LOG' ) || ! WP_COMMUNITY_EVENTS_LOG ), since PHP evaluates undefined constants as a string with the name as the value (i.e., 'WP_COMMUNITY_EVENTS_LOG'), which then evaluates to true.

Yes, that is true... I was giving a short non-functional concept. :)

Glad to see the removal of the excess logging and/or filter overhead is likely to get baked into core soon.

Last edited 21 months ago by charlestonsw (previous) (diff)
Note: See TracTickets for help on using tickets.