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

Improve opentracing for incoming HTTP requests #11618

Merged
merged 5 commits into from
Dec 20, 2021
Merged

Improve opentracing for incoming HTTP requests #11618

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c76d39
remove `start_active_span_from_request`
richvdh Dec 17, 2021
c666b5b
Remove redundant tags from `incoming-federation-request`
richvdh Dec 17, 2021
dcb6c6a
Leave request spans open until the request completes
richvdh Dec 18, 2021
d44f4eb
opentracing logs for HTTP request events
richvdh Dec 18, 2021
214b570
changelog
richvdh Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 8 additions & 20 deletions synapse/federation/transport/server/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@
from synapse.http.server import HttpServer, ServletCallback
from synapse.http.servlet import parse_json_object_from_request
from synapse.http.site import SynapseRequest
from synapse.logging import opentracing
from synapse.logging.context import run_in_background
from synapse.logging.opentracing import (
SynapseTags,
set_tag,
span_context_from_request,
start_active_span,
tags,
start_active_span_follows_from,
whitelisted_homeserver,
)
from synapse.server import HomeServer
Expand Down Expand Up @@ -279,29 +277,19 @@ async def new_func(
logger.warning("authenticate_request failed: %s", e)
raise

request_tags = {
SynapseTags.REQUEST_ID: request.get_request_id(),
tags.SPAN_KIND: tags.SPAN_KIND_RPC_SERVER,
tags.HTTP_METHOD: request.get_method(),
tags.HTTP_URL: request.get_redacted_uri(),
tags.PEER_HOST_IPV6: request.getClientIP(),
"authenticated_entity": origin,
"servlet_name": request.request_metrics.name,
}

# Only accept the span context if the origin is authenticated
# and whitelisted
# update the active opentracing span with the authenticated entity
set_tag("authenticated_entity", origin)

# if the origin is authenticated and whitelisted, link to its span context
context = None
if origin and whitelisted_homeserver(origin):
context = span_context_from_request(request)

scope = start_active_span(
"incoming-federation-request", child_of=context, tags=request_tags
scope = start_active_span_follows_from(
"incoming-federation-request", contexts=(context,) if context else ()
)
Copy link
Member Author

Choose a reason for hiding this comment

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

this changes the "incoming-federation-request" span so that its parent is no longer the outgoing request of the remote server, but rather the regular per-incoming-request span that is started in AsyncResource. Instead, the outgoing request span is set as a "reference" on the incoming request span.

Given that it's extremely unusual for us to have a single Jaeger instance which contains two federating servers, this seems by far the more useful approach.


with scope:
opentracing.inject_response_headers(request.responseHeaders)
Copy link
Member

Choose a reason for hiding this comment

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

For my own reference, this is also called via trace_servlet which is used by _AsyncResource which (I think) is an eventual super-class of all federation servlets, so we were doing work twice when we didn't need to?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that's right. Sorry I didn't make it clear!


if origin and self.RATELIMIT:
with ratelimiter.ratelimit(origin) as d:
await d
Expand Down