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

Remove unused code from synapse.logging.utils #7897

Merged
merged 3 commits into from
Jul 20, 2020
Merged

Remove unused code from synapse.logging.utils #7897

merged 3 commits into from
Jul 20, 2020

Conversation

tirkarthi
Copy link
Contributor

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Fixes #7896

@tirkarthi
Use time.perf_counter instead of time.clock for Python 3.8 compatibil…
8a91a06 
…ity.

Signed-off-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
@clokep clokep requested a review from a team July 20, 2020 11:08
@anoadragon453
Copy link
Member

Thanks! I was wondering why this wasn't breaking everything though as lots of people run Synapse on Python 3.8. From what I can tell, it looks like time_function and a few other methods in synapse/logging/utils.py aren't used anywhere.

As such, it may be a better idea to modify this PR to remove this/these methods instead :)

@tirkarthi
Copy link
Contributor Author

yes, only log_function seems to be used. Shall I just remove time_function alone? I am not sure if these are documented as part of the API to be used by someone.

rg 'logging.utils'                      
contrib/experiments/test_messaging.py
39:# from synapse.logging.utils import log_function

synapse/state/__init__.py
31:from synapse.logging.utils import log_function

synapse/storage/data_stores/main/events.py
33:from synapse.logging.utils import log_function

synapse/handlers/federation.py
64:from synapse.logging.utils import log_function

synapse/handlers/events.py
23:from synapse.logging.utils import log_function

synapse/handlers/presence.py
39:from synapse.logging.utils import log_function

synapse/federation/federation_server.py
60:from synapse.logging.utils import log_function

synapse/federation/federation_client.py
56:from synapse.logging.utils import log_function

synapse/federation/persistence.py
24:from synapse.logging.utils import log_function

synapse/federation/transport/client.py
30:from synapse.logging.utils import log_function

synapse/notifier.py
29:from synapse.logging.utils import log_function

@clokep
Copy link
Member

clokep commented Jul 20, 2020

yes, only log_function seems to be used. Shall I just remove time_function alone? I am not sure if these are documented as part of the API to be used by someone.

I don't believe these are meant to be public (although synapse.logging.context definitely is). I think time_function, trace_function, get_previous_frames, and get_previous_frame can all be removed. You'll also need to update the newsfragment to mention removing unused code instead!

@clokep clokep removed the request for review from a team July 20, 2020 17:36
@clokep clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 20, 2020
@tirkarthi
Copy link
Contributor Author

Thanks @clokep I removed the mentioned functions and added a misc note.

@clokep
Copy link
Member

clokep commented Jul 20, 2020

@tirkarthi Looks good, but linting is failing! You can test this locally by running ./scripts-dev/lint.sh (see https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.md#code-style).

@clokep clokep changed the title Use time.perf_counter instead of time.clock for Python 3.8 compatibility Remove unused code from synapse.logging.utils Jul 20, 2020
@tirkarthi
Copy link
Contributor Author

Removed unused imports. Thanks.

@clokep clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 20, 2020
@clokep clokep requested a review from a team July 20, 2020 18:58
clokep
clokep approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for finding this.

@clokep clokep merged commit 5662e2b into matrix-org:develop Jul 20, 2020
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit 'de119063f' into anoa/dinsic_release_1_18_x
a567e76 
* commit 'de119063f': (31 commits)
  Convert room list handler to async/await. (#7912)
  Element CSS and logo in email templates (#7919)
  Lint the contrib/ directory in CI and linting scripts, add synctl to linting script (#7914)
  Remove unused code from synapse.logging.utils. (#7897)
  Fix a typo in the sample config. (#7890)
  Fix deprecation warning: import ABC from collections.abc (#7892)
  Change sample config's postgres user to synapse_user (#7889)
  Fix deprecation warning due to invalid escape sequences (#7895)
  Remove Ubuntu Eoan that is now EOL (#7888)
  Fix the trace function for async functions. (#7872)
  Add help for creating a user via docker (#7885)
  Switch to Debian:Slim from Alpine for the docker image (#7839)
  Stop using 'device_max_stream_id' (#7882)
  Fix TypeError in synapse.notifier (#7880)
  Add a default limit (of 100) to get/sync operations. (#7858)
  Change "unknown room ver" logging to warning. (#7881)
  Convert device handler to async/await (#7871)
  Convert synapse.app to async/await. (#7868)
  Convert _base, profile, and _receipts handlers to async/await (#7860)
  Add admin endpoint to get members in a room. (#7842)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

time.clock has been deprecated and removed in python 3.8 in favor of time.perf_counter/process_time
3 participants
@tirkarthi @anoadragon453 @clokep