WSGI fixes

[Oberon00]

• Fix http.url (WSGI integration does nonstandard split between http.host and http.url #143)
• Don't delay calling wrapped app (would be nicer if No way to use_span withouth ending it. #147 was fixed; see Implement WSGI middleware integration. #84 (comment) and following)

[a-feld]

@Oberon00 This looks great! I have some questions / suggestions, thanks for taking the time to improve this!

[a-feld]

Ah I didn't realize that http.host was nonstandard! Thanks for catching this!
Given that it's not standard, should we just remove this? It seems like a lot of overhead per request to compute the host string.

[Oberon00]

I don't think the calculation for host is particularly expensive and it could probably even be optimized. I'd wait until the data formats are reworked (this is something that will most likely happen).

[Oberon00]

I shortened & hopefully optimized it.

[a-feld]

Also, are these cases that we have to cover?

        self.environ["RAW_URI"] = "/?"
        self.environ["RAW_URI"] = "http:///?"
        self.environ["RAW_URI"] = "http://?"

These cases seem to add a lot of complexity. I'm curious to know which WSGI servers write these RAW_URI strings?

[a-feld]

Looking more at this, in order to be completely spec compliant it feels like calling url = wsgiref_util.request_uri(environ) on every request would probably be easiest rather than playing with RAW_URI and REQUEST_URI as an optimization. What are your thoughts on this?

[Oberon00]

On the one hand, I think using RAW_URI et al is valuable because it catches things like http://myhost/? vs http://myhost/, a difference that should not matter but could. And since one of the prime use cases of tracing is debugging, I think exactly these edge cases are interesting to handle. But on the other hand, I might have gone overboard with my handling of malformed/very unusual raw URIs. Maybe there is a middle ground to aim for.

[reyang]

Not sure if we can get something like http://foobar.org/abc#CrazyFragment and how do we plan to handle it.
I swear that I've seen something like this from server side (which means both client and server are super buggy).

[Oberon00]

If we get that all in RAW_URL, we'll set it to the http.url attribute as-is. If we get invalid data, we are not in the business to normalize it, instead we should make it visible.

[Oberon00]

I simplified it:

• If the raw URL starts with / we assume it is relative and prepend scheme://host
• Otherwise, if the url starts with "scheme:", we use it as-is
• Otherwise we assume a bogus value and fall back to wsgiref.util.request_uri.

[c24t]

Why raise again here instead of making this a finally block?

[reyang]

I guess it was because the assumption that Span.end() gets called in iter_result when there is no exception.
This piece of code is a bit risky and hard to maintain IMHO.

[reyang]

If we decided to go with this approach, I'll be okay if we can put a comment like this :)

# ATTENTION!!! HIGH VOLTAGE!!!

[Oberon00]

If we can determine if a span has ended, then finally: if not ended: end() would be good.

[Oberon00]

If we decided to go with this approach

I'm all for a better approach if somebody finds one that still doesn't delay the app invocation.

See also: #148 (comment)

I can't think of a way that doesn't have a slim chance of either ending the span twice or not ending it at all.

[c24t]

By making __call__ a generator, we only start the span once the server starts iterating.

That's very subtle, good catch @Oberon00.

This LGTM, but I agree that playing weird URI whack-a-mole may not be worth the effort.

[reyang]

This seems to be risky.
According to the API, implementation could raise exception if span.end() is called twice.

If an exception happened right before iter_result returns (but after span.end() is called), we end up calling end() twice and kill the user app due to unwanted exception.

[c24t]

We may want to change the API so that we don't raise on duplicate calls to end, which seems like a good idea regardless of this PR.

I've got an open PR in specs about suppressing errors in the API. The goal is to avoid exactly this kind of thing -- crashing the application because we're using the instrumentation layer wrong.

[Oberon00]

Seems there is no way to do it right with the current API then.

[reyang]

Probably leave a TODO comment for now.

[Oberon00]

BTW, the current SDK only logs a warning, which is fine because having end called twice here should really be an edge case.

[Oberon00]

For end in particular the spec already says that calling it multiple times is fine, see #154 (comment)

[Oberon00]

The test failure seems to be due to #147. Since this is a real problem, a fix for that issue is required before merging this PR.

Fixed by #154, I rebased this PR.

[Oberon00]

@a-feld Is there still something you want to have changed?

[a-feld]

Sorry for the delay @Oberon00 , I'm giving this another once over! 🙏
I'll also dismiss my review, this can be changed incrementally anyway over time if required 😄

[a-feld]

This logic appears to always attach port 80 in the presence of http. Is that intentional?

environ = {"SERVER_NAME": "example.com", "SERVER_PORT": "80", "wsgi.url_scheme": "http"}

host = environ.get("HTTP_HOST")
if not host:
    host = environ["SERVER_NAME"]
    port = environ["SERVER_PORT"]
    if port != "80" and environ["wsgi.url_scheme"] == "http" or port != "443":
        host += ":" + port


print(host)

example.com:80

The logic is equivalent to:
port != "443" or environ["wsgi.url_scheme"] == "http" and port != "80" so all http ports will be reported?

[Oberon00]

Certainly not. I thought I had a unit test against exactly that. Will investigate.

dismiss review

[a-feld]

@Oberon00 Thanks for putting this together, from what I can see this looks great! I'll approve this now assuming that the http port investigation will conclude!

[Oberon00]

@reyang I intented to fix the port issue before merging this, but oh well, I can also file a follow-up PR.