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

Fix error reporting when using opentracing.trace #7961

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Jul 27, 2020

In particular start_active_span returns a scope, which is meant to be a context manager that returns itself, but it didn't. This fixes that.

We also clean some other stuff up, we don't actually need to set error=True tag manually, that happens for us, and there is no point having the if opentracing guard there given we both have it up a layer and start_active_span returns a null context manager anyway.

Broke in #7872

Stack trace:

AttributeError: 'NoneType' object has no attribute 'span'
  File "synapse/http/server.py", line 228, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "synapse/http/server.py", line 399, in _async_render
    callback_return = await raw_callback_return
  File "synapse/rest/client/v2_alpha/room_keys.py", line 134, in on_PUT
    ret = await self.e2e_room_keys_handler.upload_room_keys(user_id, version, body)
  File "synapse/logging/opentracing.py", line 747, in _trace_inner
    scope.span.set_tag(tags.ERROR, True)

@erikjohnston erikjohnston requested a review from a team July 27, 2020 14:58
@@ -0,0 +1 @@
Fix sometimes returning 500 instead of the actual error response when using opentracing.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Fix sometimes returning 500 instead of the actual error response when using opentracing.
Fix a bug which could cause `500 Internal Server Error` to be returned instead of the actual response when an error was returned from REST APIs and opentracing was enabled..

@@ -0,0 +1 @@
Fix a long standing bug where the tracing of async functions with opentracing was broken.
Copy link
Member

Choose a reason for hiding this comment

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

oh this is fine too

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

Comment on lines -740 to 741
if opentracing is None:
with start_active_span(_opname):
return await func(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

I tried to remove this earlier and it caused issues: #7872

I think the case this protects against is having opentracing installed, but disabled via the config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you also remove the explicit scope.span.set_tag(...)?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, no. That makes sense. 👍

@erikjohnston erikjohnston merged commit 1ef9efc into develop Jul 27, 2020
@erikjohnston erikjohnston deleted the erikj/opentracing branch July 27, 2020 15:20
anoadragon453 added a commit that referenced this pull request Jul 28, 2020
…rove_test_times

* 'develop' of github.com:matrix-org/synapse: (148 commits)
  Add script for finding files with unix line terminators (#7965)
  Convert the remaining media repo code to async / await. (#7947)
  Convert a synapse.events to async/await. (#7949)
  Convert groups and visibility code to async / await. (#7951)
  Convert push to async/await. (#7948)
  update changelog
  1.18.0rc1
  Fix error reporting when using `opentracing.trace` (#7961)
  Fix typing replication not being handled on master (#7959)
  Remove hacky error handling for inlineDeferreds. (#7950)
  Convert tests/rest/admin/test_room.py to unix file endings (#7953)
  Support oEmbed for media previews. (#7920)
  Convert state resolution to async/await (#7942)
  Fix up types and comments that refer to Deferreds. (#7945)
  Do not convert async functions to Deferreds in the interactive_auth_handler (#7944)
  Convert more of the media code to async/await (#7873)
  Return an empty body for OPTIONS requests. (#7886)
  Downgrade warning on client disconnect to INFO (#7928)
  Convert presence handler helpers to async/await. (#7939)
  Update the auth providers to be async. (#7935)
  ...
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'f88c48f3b':
  1.18.0rc1
  Fix error reporting when using `opentracing.trace` (#7961)
  Fix typing replication not being handled on master (#7959)
  Remove hacky error handling for inlineDeferreds. (#7950)
  Convert tests/rest/admin/test_room.py to unix file endings (#7953)
  Support oEmbed for media previews. (#7920)
  Convert state resolution to async/await (#7942)
  Fix up types and comments that refer to Deferreds. (#7945)
  Do not convert async functions to Deferreds in the interactive_auth_handler (#7944)
  Convert more of the media code to async/await (#7873)
  Return an empty body for OPTIONS requests. (#7886)
  Downgrade warning on client disconnect to INFO (#7928)
  Convert presence handler helpers to async/await. (#7939)
  Update the auth providers to be async. (#7935)
  Put a cache on `/state_ids` (#7931)
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