Fix `@tag_args` being off-by-one (ahead) #13452

MadLittleMods · 2022-08-04T01:55:01Z

Fix @tag_args being off-by-one (ahead)

Before	After

Example:

argspec.args=[
  'self',
  'room_id'
]

args=(
  <synapse.storage.databases.main.DataStore object at 0x10d0b8d00>,
  '!HBehERstyQBxyJDLfR:my.synapse.server'
)

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.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
~~Pull request includes a sign off~~
Code style is correct
(run the linters)

Example: ``` argspec.args=[ 'self', 'room_id' ] args=( <synapse.storage.databases.main.DataStore object at 0x10d0b8d00>, '!HBehERstyQBxyJDLfR:my.synapse.server' ) ```

MadLittleMods · 2022-08-04T02:03:05Z

synapse/logging/opentracing.py

        for i, arg in enumerate(argspec.args[1:]):
-            set_tag("ARG_" + arg, str(args[i]))  # type: ignore[index]
+            set_tag("ARG_" + arg, str(args[i + 1]))  # type: ignore[index]


Tests possible if you push 😓

Maybe more worth doing since it doesn't seem to behave as we assume (#13452 (comment))

synapse/logging/opentracing.py

See #13452 (comment) The previous logic is flawed and we can end up in a situation like this: ``` argspec.args=['self', 'dest', 'room_id', 'limit', 'extremities'] args=(<synapse.federation.federation_client.FederationClient object at 0x7f1651c18160>, 'hs1', '!jAEHKIubyIfuLOdfpY:hs1') ``` From this source: ```py async def backfill( self, dest: str, room_id: str, limit: int, extremities: Collection[str] ) -> Optional[List[EventBase]]: ``` And this usage: ```py events = await self._federation_client.backfill( dest, room_id, limit=limit, extremities=extremities ) ``` Causes this error: ``` synapse_main | 2022-08-04 06:13:12,051 - synapse.handlers.federation - 424 - ERROR - GET-5 - Failed to backfill from hs1 because tuple index out of range synapse_main | Traceback (most recent call last): synapse_main | File "/usr/local/lib/python3.9/site-packages/synapse/handlers/federation.py", line 392, in try_backfill synapse_main | await self._federation_event_handler.backfill( synapse_main | File "/usr/local/lib/python3.9/site-packages/synapse/logging/tracing.py", line 828, in _wrapper synapse_main | return await func(*args, **kwargs) synapse_main | File "/usr/local/lib/python3.9/site-packages/synapse/handlers/federation_event.py", line 593, in backfill synapse_main | events = await self._federation_client.backfill( synapse_main | File "/usr/local/lib/python3.9/site-packages/synapse/logging/tracing.py", line 828, in _wrapper synapse_main | return await func(*args, **kwargs) synapse_main | File "/usr/local/lib/python3.9/site-packages/synapse/logging/tracing.py", line 827, in _wrapper synapse_main | with wrapping_logic(func, *args, **kwargs): synapse_main | File "/usr/local/lib/python3.9/contextlib.py", line 119, in __enter__ synapse_main | return next(self.gen) synapse_main | File "/usr/local/lib/python3.9/site-packages/synapse/logging/tracing.py", line 922, in _wrapping_logic synapse_main | set_attribute("ARG_" + arg, str(args[i + 1])) # type: ignore[index] synapse_main | IndexError: tuple index out of range ```

synapse/logging/opentracing.py

DMRobertson

Thanks! Other than the docstring suggestion, the rest of the comments are just thoughts in passing.

synapse/logging/opentracing.py

See #13452 (comment)

DMRobertson

LGTM (though this was a lot more subtle/confusing than I thought at first glance. Sean, would you mind giving this another pair of eyes? (You've already caught one error of mine on this review!)

lgtm, but I'd like another pair of eyes please!

squahtx · 2022-08-04T17:24:27Z

Sean, would you mind giving this another pair of eyes?

Sure. It does look like you've got everything covered!

squahtx

lgtm!

The biggest source of confusion for me was whether func, args and/or argspec had a self parameter. It sounds like they all do, so maybe that confusion was unwarranted.

As an alternative to indexing argspec.args, we might be able to do something with zip()? I think the current code is fine anyway.

Conditionally ignoring the first argument if it's called self or cls sounds good to me.

MadLittleMods · 2022-08-04T19:30:23Z

Thanks for the review @DMRobertson and @squahtx 🦨

@andrewdoh

Synapse 1.65.0 (2022-08-16) =========================== No significant changes since 1.65.0rc2. Synapse 1.65.0rc2 (2022-08-11) ============================== Internal Changes ---------------- - Revert 'Remove the unspecced `room_id` field in the `/hierarchy` response. ([\matrix-org#13365](matrix-org#13365))' to give more time for clients to update. ([\matrix-org#13501](matrix-org#13501)) Synapse 1.65.0rc1 (2022-08-09) ============================== Features -------- - Add support for stable prefixes for [MSC2285 (private read receipts)](matrix-org/matrix-spec-proposals#2285). ([\matrix-org#13273](matrix-org#13273)) - Add new unstable error codes `ORG.MATRIX.MSC3848.ALREADY_JOINED`, `ORG.MATRIX.MSC3848.NOT_JOINED`, and `ORG.MATRIX.MSC3848.INSUFFICIENT_POWER` described in [MSC3848](matrix-org/matrix-spec-proposals#3848). ([\matrix-org#13343](matrix-org#13343)) - Use stable prefixes for [MSC3827](matrix-org/matrix-spec-proposals#3827). ([\matrix-org#13370](matrix-org#13370)) - Add a new module API method to translate a room alias into a room ID. ([\matrix-org#13428](matrix-org#13428)) - Add a new module API method to create a room. ([\matrix-org#13429](matrix-org#13429)) - Add remote join capability to the module API's `update_room_membership` method (in a backwards compatible manner). ([\matrix-org#13441](matrix-org#13441)) Bugfixes -------- - Update the version of the LDAP3 auth provider module included in the `matrixdotorg/synapse` DockerHub images and the Debian packages hosted on packages.matrix.org to 0.2.2. This version fixes a regression in the module. ([\matrix-org#13470](matrix-org#13470)) - Fix a bug introduced in Synapse v1.41.0 where the `/hierarchy` API returned non-standard information (a `room_id` field under each entry in `children_state`). ([\matrix-org#13365](matrix-org#13365)) - Fix a bug introduced in Synapse 0.24.0 that would respond with the wrong error status code to `/joined_members` requests when the requester is not a current member of the room. Contributed by @andrewdoh. ([\matrix-org#13374](matrix-org#13374)) - Fix bug in handling of typing events for appservices. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13392](matrix-org#13392)) - Fix a bug introduced in Synapse 1.57.0 where rooms listed in `exclude_rooms_from_sync` in the configuration file would not be properly excluded from incremental syncs. ([\matrix-org#13408](matrix-org#13408)) - Fix a bug in the experimental faster-room-joins support which could cause it to get stuck in an infinite loop. ([\matrix-org#13353](matrix-org#13353)) - Faster room joins: fix a bug which caused rejected events to become un-rejected during state syncing. ([\matrix-org#13413](matrix-org#13413)) - Faster room joins: fix error when running out of servers to sync partial state with, so that Synapse raises the intended error instead. ([\matrix-org#13432](matrix-org#13432)) Updates to the Docker image --------------------------- - Make Docker images build on armv7 by installing cryptography dependencies in the 'requirements' stage. Contributed by Jasper Spaans. ([\matrix-org#13372](matrix-org#13372)) Improved Documentation ---------------------- - Update the 'registration tokens' page to acknowledge that the relevant MSC was merged into version 1.2 of the Matrix specification. Contributed by @moan0s. ([\matrix-org#11897](matrix-org#11897)) - Document which HTTP resources support gzip compression. ([\matrix-org#13221](matrix-org#13221)) - Add steps describing how to elevate an existing user to administrator by manipulating the database. ([\matrix-org#13230](matrix-org#13230)) - Fix wrong headline for `url_preview_accept_language` in documentation. ([\matrix-org#13437](matrix-org#13437)) - Remove redundant 'Contents' section from the Configuration Manual. Contributed by @dklimpel. ([\matrix-org#13438](matrix-org#13438)) - Update documentation for config setting `macaroon_secret_key`. ([\matrix-org#13443](matrix-org#13443)) - Update outdated information on `sso_mapping_providers` documentation. ([\matrix-org#13449](matrix-org#13449)) - Fix example code in module documentation of `password_auth_provider_callbacks`. ([\matrix-org#13450](matrix-org#13450)) - Make the configuration for the cache clearer. ([\matrix-org#13481](matrix-org#13481)) Internal Changes ---------------- - Extend the release script to automatically push a new SyTest branch, rather than having that be a manual process. ([\matrix-org#12978](matrix-org#12978)) - Make minor clarifications to the error messages given when we fail to join a room via any server. ([\matrix-org#13160](matrix-org#13160)) - Enable Complement CI tests in the 'latest deps' test run. ([\matrix-org#13213](matrix-org#13213)) - Fix long-standing bugged logic which was never hit in `get_pdu` asking every remote destination even after it finds an event. ([\matrix-org#13346](matrix-org#13346)) - Faster room joins: avoid blocking when pulling events with partially missing prev events. ([\matrix-org#13355](matrix-org#13355)) - Instrument `/messages` for understandable traces in Jaeger. ([\matrix-org#13368](matrix-org#13368)) - Remove an unused argument to `get_relations_for_event`. ([\matrix-org#13383](matrix-org#13383)) - Add a `merge-back` command to the release script, which automates merging the correct branches after a release. ([\matrix-org#13393](matrix-org#13393)) - Adding missing type hints to tests. ([\matrix-org#13397](matrix-org#13397)) - Faster Room Joins: don't leave a stuck room partial state flag if the join fails. ([\matrix-org#13403](matrix-org#13403)) - Refactor `_resolve_state_at_missing_prevs` to compute an `EventContext` instead. ([\matrix-org#13404](matrix-org#13404), [\matrix-org#13431](matrix-org#13431)) - Faster Room Joins: prevent Synapse from answering federated join requests for a room which it has not fully joined yet. ([\matrix-org#13416](matrix-org#13416)) - Re-enable running Complement tests against Synapse with workers. ([\matrix-org#13420](matrix-org#13420)) - Prevent unnecessary lookups to any external `get_event` cache. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13435](matrix-org#13435)) - Add some tracing to give more insight into local room joins. ([\matrix-org#13439](matrix-org#13439)) - Rename class `RateLimitConfig` to `RatelimitSettings` and `FederationRateLimitConfig` to `FederationRatelimitSettings`. ([\matrix-org#13442](matrix-org#13442)) - Add some comments about how event push actions are stored. ([\matrix-org#13445](matrix-org#13445), [\matrix-org#13455](matrix-org#13455)) - Improve rebuild speed for the "synapse-workers" docker image. ([\matrix-org#13447](matrix-org#13447)) - Fix `@tag_args` being off-by-one with the arguments when tagging a span (tracing). ([\matrix-org#13452](matrix-org#13452)) - Update type of `EventContext.rejected`. ([\matrix-org#13460](matrix-org#13460)) - Use literals in place of `HTTPStatus` constants in tests. ([\matrix-org#13463](matrix-org#13463), [\matrix-org#13469](matrix-org#13469)) - Correct a misnamed argument in state res v2 internals. ([\matrix-org#13467](matrix-org#13467))

MadLittleMods added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Aug 4, 2022

MadLittleMods added 2 commits August 3, 2022 21:00

Fix @tag_args being off-by-one (ahead)

948090f

Example: ``` argspec.args=[ 'self', 'room_id' ] args=( <synapse.storage.databases.main.DataStore object at 0x10d0b8d00>, '!HBehERstyQBxyJDLfR:my.synapse.server' ) ```

Add changelog

a988d5f

MadLittleMods force-pushed the madlittlemods/fix-@tag_args-off-by-one branch from b508f93 to a988d5f Compare August 4, 2022 02:01

MadLittleMods commented Aug 4, 2022

View reviewed changes

MadLittleMods marked this pull request as ready for review August 4, 2022 02:04

MadLittleMods requested a review from a team as a code owner August 4, 2022 02:04

MadLittleMods commented Aug 4, 2022

View reviewed changes

synapse/logging/opentracing.py Outdated Show resolved Hide resolved

MadLittleMods mentioned this pull request Aug 4, 2022

Allow use of both @trace and @tag_args stacked on the same function #13453

Merged

4 tasks

MadLittleMods commented Aug 4, 2022

View reviewed changes

synapse/logging/opentracing.py Show resolved Hide resolved

DMRobertson reviewed Aug 4, 2022

View reviewed changes

synapse/logging/opentracing.py Outdated Show resolved Hide resolved

DMRobertson reviewed Aug 4, 2022

View reviewed changes

synapse/logging/opentracing.py Outdated Show resolved Hide resolved

DMRobertson previously requested changes Aug 4, 2022

View reviewed changes

DMRobertson reviewed Aug 4, 2022

View reviewed changes

synapse/logging/opentracing.py Outdated Show resolved Hide resolved

MadLittleMods added 3 commits August 4, 2022 11:25

Align indexes

7e34d6d

See #13452 (comment)

Note caveat on the kind of function we expect to tag

7dcf6f7

See #13452 (comment)

Fix lints

3389a23

MadLittleMods requested a review from DMRobertson August 4, 2022 16:49

Document potential future upgrades

4152613

See #13452 (comment)

DMRobertson reviewed Aug 4, 2022

View reviewed changes

DMRobertson requested a review from squahtx August 4, 2022 17:00

squahtx approved these changes Aug 4, 2022

View reviewed changes

MadLittleMods merged commit 860fdd9 into develop Aug 4, 2022

MadLittleMods deleted the madlittlemods/fix-@tag_args-off-by-one branch August 4, 2022 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `@tag_args` being off-by-one (ahead) #13452

Fix `@tag_args` being off-by-one (ahead) #13452

MadLittleMods commented Aug 4, 2022 •

edited

Loading

MadLittleMods Aug 4, 2022

MadLittleMods Aug 4, 2022

DMRobertson left a comment

DMRobertson left a comment

squahtx commented Aug 4, 2022

squahtx left a comment

MadLittleMods commented Aug 4, 2022

Fix @tag_args being off-by-one (ahead) #13452

Fix @tag_args being off-by-one (ahead) #13452

Conversation

MadLittleMods commented Aug 4, 2022 • edited Loading

Pull Request Checklist

MadLittleMods Aug 4, 2022

Choose a reason for hiding this comment

MadLittleMods Aug 4, 2022

Choose a reason for hiding this comment

DMRobertson left a comment

Choose a reason for hiding this comment

DMRobertson left a comment

Choose a reason for hiding this comment

squahtx commented Aug 4, 2022

squahtx left a comment

Choose a reason for hiding this comment

MadLittleMods commented Aug 4, 2022

Fix `@tag_args` being off-by-one (ahead) #13452

Fix `@tag_args` being off-by-one (ahead) #13452

MadLittleMods commented Aug 4, 2022 •

edited

Loading