Skip to content

Conversation

@gravelg
Copy link
Contributor

@gravelg gravelg commented Dec 14, 2017

If the flask app has a before_request method that shortcuts request execution, the request will not be present in the _current_spans dict but the tracer will still try to finish the span. Adding this default prevents an exception on those calls.

@ror6ax
Copy link
Contributor

ror6ax commented May 29, 2018

Hi @gravelg , can you please kindly rebase off master?

@gravelg
Copy link
Contributor Author

gravelg commented May 29, 2018

Sure thing, rebased!

@ror6ax ror6ax requested a review from carlosalberto May 29, 2018 14:32
@ror6ax
Copy link
Contributor

ror6ax commented May 29, 2018

Seems fine with me. Let's see what Carlos thinks.

@carlosalberto
Copy link
Contributor

Hey @gravelg thanks for the PR - this definitely looks fine (we do the same for the Django instrumentation, for example).

But I'm wondering, if the scenario you talk about, is probable another bug (in our side)? If you have a simple test case, I'd love to see it (as to me it sounds you are instrumenting your code and also wrapping some handler of yours through @app.before_request(), which we should handle just fine).

Let me know and I will check ASAP ;)

@gravelg
Copy link
Contributor Author

gravelg commented May 31, 2018

Hey @carlosalberto, I don't think I'm doing something out of the ordinary, I just have a global before_request method like this:

@app.before_request
    def _before_request():
        # max 10mb body size
        if request.content_length is not None and request.content_length > 10485760:
            return make_response(jsonify({"error_code": "REQUEST_ENTITY_TOO_LARGE",
                                          "error_message": "Request of size {} larger than maximum size of {}".format(
                                              request.content_length, 10485760)}), 413)

This follows the flask docs that say that if a before_request method returns a value, it's interpreted as the return value for the call. This is the case that my PR is trying to fix.

@carlosalberto
Copy link
Contributor

Hey @gravelg thanks for the reply and sorry for the delayed answer.

Nice, I was afraid for a second there was a bug in the actual instrumentation (our side). Would you mind putting a one-line comment (in the code) regarding the situation/condition? After that we can merge it (I'd love to have a note for future maintainers to know in which case we are not having Spans) ;)

Thanks again for the PR (and the patience)!

@gravelg
Copy link
Contributor Author

gravelg commented Jun 8, 2018

Sure thing, I've added a comment. Let me know if this is what you were looking for.

@carlosalberto carlosalberto merged commit c003984 into opentracing-contrib:master Jun 8, 2018
@carlosalberto
Copy link
Contributor

Lovely, thanks again for the PR and for the patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants