Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use url.rule instead of request.endpoint for span name flask instrumentation #1260

Merged
merged 5 commits into from
Oct 21, 2020

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Oct 20, 2020

From the title, we don't want to be using endpoint for the span name. See specs for span name.

Use url.rule instead if exists. If not, default to HTTP

@lzchen lzchen requested review from a team, codeboten and aabmass and removed request for a team October 20, 2020 18:44
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Is this PR ready for review or a draft?

@@ -110,7 +110,7 @@ def _before_request():
return

environ = flask.request.environ
span_name = flask.request.endpoint or otel_wsgi.get_default_span_name(
span_name = flask.request.url_rule.rule or otel_wsgi.get_default_span_name(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not familiar enough with flask. Is url_rule always available? What if the rule is not defined and the request is a 404?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not always available. If it's not, we default to the wsgi implementation which is HTTP +

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this raise attribute error if request.url_rule is not always present? or is url_rule always present but rule can be None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a try catch for attribute error in my latest commit.

@codeboten codeboten added the instrumentation Related to the instrumentation of third party libraries or frameworks label Oct 20, 2020
@lzchen
Copy link
Contributor Author

lzchen commented Oct 20, 2020

@codeboten
should be ready

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This looks good, one non-blocking question

except AttributeError:
pass
if span_name is None:
span_name = otel_wsgi.get_default_span_name(environ)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a test that exerts this code path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test_404 test covers this.

@lzchen lzchen merged commit bda3fbb into open-telemetry:master Oct 21, 2020
@lzchen lzchen deleted the flask branch October 21, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instrumentation Related to the instrumentation of third party libraries or frameworks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants