Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove non-dedicated logging options and command line arguments #5678

Merged
merged 7 commits into from
Jul 18, 2019

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Jul 13, 2019

This simplifies things in preparation for the integration of structured logging, namely:

  • Removing the deprecated command line arguments (which survived a purge way back in 0.9.0)
  • Removing the functionality of the obsolete log_file and verbose settings
  • Cleaning up the amount of things that use sighups

…ging code in preparation for structured logging changes
@hawkowl hawkowl self-assigned this Jul 13, 2019
@codecov
Copy link

codecov bot commented Jul 13, 2019

Codecov Report

Merging #5678 into develop will decrease coverage by 0.54%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #5678      +/-   ##
===========================================
- Coverage    63.27%   62.73%   -0.55%     
===========================================
  Files          331      331              
  Lines        36058    36002      -56     
  Branches      5936     5923      -13     
===========================================
- Hits         22815    22585     -230     
- Misses       11611    11769     +158     
- Partials      1632     1648      +16

@codecov
Copy link

codecov bot commented Jul 13, 2019

Codecov Report

Merging #5678 into develop will decrease coverage by 0.04%.
The diff coverage is 16.66%.

@@             Coverage Diff             @@
##           develop    #5678      +/-   ##
===========================================
- Coverage    63.27%   63.22%   -0.05%     
===========================================
  Files          331      331              
  Lines        36058    36005      -53     
  Branches      5936     5923      -13     
===========================================
- Hits         22815    22764      -51     
- Misses       11611    11615       +4     
+ Partials      1632     1626       -6

@hawkowl hawkowl requested a review from a team July 13, 2019 15:41
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regularly use --no-redirect-stdio; in fact I think that redirecting stdio at the python level is a misfeature (see also #1539 (comment)). Please can it not go away?

@hawkowl
Copy link
Contributor Author

hawkowl commented Jul 15, 2019

@richvdh if we need to print to true stdout or stderr (say in a situation where the logger doesn't work, or we want to perform a core dump, or other things), we should use the sys.__stdout__ and sys.__stderr__ references, which always reference true stdout/stderr, and will not be caught by the logging system.

Allowing regular print() calls to go to true stdout can mess up many things, especially when the logs being output there is in a machine-readable format (JSON, etc). Unless we explicitly want true stdout (by using print("something went wrong!", file=sys.__stdout__, flush=True), everything should go through the logging system without exception -- especially considering that in a cloud environment (SHHS), the logging system will be primarily forwarding structured logs over TCP, and information going to stdout may not be caught by that if you don't have something like fluentd set up to capture Docker stdout (which we're doing, but not everyone might).

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. I'm sure we'll find someone who is traumatised by the removal of these options, but I guess that's a battle we'll have to resign ourselves to.


logging.getLogger("synapse.storage.SQL").setLevel(level_for_storage)
logger.setLevel(logging.INFO)
logging.getLogger("synapse.storage.SQL").setLevel(logging.INFO)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is probably redundant, since it's the same as the root logger?

@hawkowl hawkowl merged commit 6a85cb5 into develop Jul 18, 2019
@hawkowl hawkowl deleted the hawkowl/prune-logging-options branch July 18, 2019 15:40
babolivier added a commit that referenced this pull request Aug 15, 2019
Synapse 1.3.0 (2019-08-15)
==========================

Bugfixes
--------

- Fix 500 Internal Server Error on `publicRooms` when the public room list was
  cached. ([\#5851](#5851))

Synapse 1.3.0rc1 (2019-08-13)
==========================

Features
--------

- Use `M_USER_DEACTIVATED` instead of `M_UNKNOWN` for errcode when a deactivated user attempts to login. ([\#5686](#5686))
- Add sd_notify hooks to ease systemd integration and allows usage of Type=Notify. ([\#5732](#5732))
- Synapse will no longer serve any media repo admin endpoints when `enable_media_repo` is set to False in the configuration. If a media repo worker is used, the admin APIs relating to the media repo will be served from it instead. ([\#5754](#5754), [\#5848](#5848))
- Synapse can now be configured to not join remote rooms of a given "complexity" (currently, state events) over federation. This option can be used to prevent adverse performance on resource-constrained homeservers. ([\#5783](#5783))
- Allow defining HTML templates to serve the user on account renewal attempt when using the account validity feature. ([\#5807](#5807))

Bugfixes
--------

- Fix UISIs during homeserver outage. ([\#5693](#5693), [\#5789](#5789))
- Fix stack overflow in server key lookup code. ([\#5724](#5724))
- start.sh no longer uses deprecated cli option. ([\#5725](#5725))
- Log when we receive an event receipt from an unexpected origin. ([\#5743](#5743))
- Fix debian packaging scripts to correctly build sid packages. ([\#5775](#5775))
- Correctly handle redactions of redactions. ([\#5788](#5788))
- Return 404 instead of 403 when accessing /rooms/{roomId}/event/{eventId} for an event without the appropriate permissions. ([\#5798](#5798))
- Fix check that tombstone is a state event in push rules. ([\#5804](#5804))
- Fix error when trying to login as a deactivated user when using a worker to handle login. ([\#5806](#5806))
- Fix bug where user `/sync` stream could get wedged in rare circumstances. ([\#5825](#5825))
- The purge_remote_media.sh script was fixed. ([\#5839](#5839))

Deprecations and Removals
-------------------------

- Synapse now no longer accepts the `-v`/`--verbose`, `-f`/`--log-file`, or `--log-config` command line flags, and removes the deprecated `verbose` and `log_file` configuration file options. Users of these options should migrate their options into the dedicated log configuration. ([\#5678](#5678), [\#5729](#5729))
- Remove non-functional 'expire_access_token' setting. ([\#5782](#5782))

Internal Changes
----------------

- Make Jaeger fully configurable. ([\#5694](#5694))
- Add precautionary measures to prevent future abuse of `window.opener` in default welcome page. ([\#5695](#5695))
- Reduce database IO usage by optimising queries for current membership. ([\#5706](#5706), [\#5738](#5738), [\#5746](#5746), [\#5752](#5752), [\#5770](#5770), [\#5774](#5774), [\#5792](#5792), [\#5793](#5793))
- Improve caching when fetching `get_filtered_current_state_ids`. ([\#5713](#5713))
- Don't accept opentracing data from clients. ([\#5715](#5715))
- Speed up PostgreSQL unit tests in CI. ([\#5717](#5717))
- Update the coding style document. ([\#5719](#5719))
- Improve database query performance when recording retry intervals for remote hosts. ([\#5720](#5720))
- Add a set of opentracing utils. ([\#5722](#5722))
- Cache result of get_version_string to reduce overhead of `/version` federation requests. ([\#5730](#5730))
- Return 'user_type' in admin API user endpoints results. ([\#5731](#5731))
- Don't package the sytest test blacklist file. ([\#5733](#5733))
- Replace uses of returnValue with plain return, as returnValue is not needed on Python 3. ([\#5736](#5736))
- Blacklist some flakey tests in worker mode. ([\#5740](#5740))
- Fix some error cases in the caching layer. ([\#5749](#5749))
- Add a prometheus metric for pending cache lookups. ([\#5750](#5750))
- Stop trying to fetch events with event_id=None. ([\#5753](#5753))
- Convert RedactionTestCase to modern test style. ([\#5768](#5768))
- Allow looping calls to be given arguments. ([\#5780](#5780))
- Set the logs emitted when checking typing and presence timeouts to DEBUG level, not INFO. ([\#5785](#5785))
- Remove DelayedCall debugging from the test suite, as it is no longer required in the vast majority of Synapse's tests. ([\#5787](#5787))
- Remove some spurious exceptions from the logs where we failed to talk to a remote server. ([\#5790](#5790))
- Improve performance when making `.well-known` requests by sharing the SSL options between requests. ([\#5794](#5794))
- Disable codecov GitHub comments on PRs. ([\#5796](#5796))
- Don't allow clients to send tombstone events that reference the room it's sent in. ([\#5801](#5801))
- Deny redactions of events sent in a different room. ([\#5802](#5802))
- Deny sending well known state types as non-state events. ([\#5805](#5805))
- Handle incorrectly encoded query params correctly by returning a 400. ([\#5808](#5808))
- Handle pusher being deleted during processing rather than logging an exception. ([\#5809](#5809))
- Return 502 not 500 when failing to reach any remote server. ([\#5810](#5810))
- Reduce global pauses in the events stream caused by expensive state resolution during persistence. ([\#5826](#5826))
- Add a lower bound to well-known lookup cache time to avoid repeated lookups. ([\#5836](#5836))
- Whitelist history visbility sytests in worker mode tests. ([\#5843](#5843))
@ara4n
Copy link
Member

ara4n commented Oct 23, 2019

lgtm. I'm sure we'll find someone who is traumatised by the removal of these options, but I guess that's a battle we'll have to resign ourselves to.

ftr, that was me, on discovering that my synapse had silently stopped logging a few months ago, which is a right pain in the ass.

please can we at least warn loudly (or break hard) when removing old config options.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants