-
Notifications
You must be signed in to change notification settings - Fork 607
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
Expand allowed versions for jinja2 instrumentation #712
Conversation
The Jinja2 API hasn't changed the functions that have been wrapped in at least 8 years. The version supported should be much more broad.
|
...ion/opentelemetry-instrumentation-jinja2/src/opentelemetry/instrumentation/jinja2/package.py
Outdated
Show resolved
Hide resolved
@owais Can you let me know if there's anything I need to do to get the checks to run? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I triggered the build just now by updating the branch, it appears to be running.
@codeboten Thanks for the pointer. I've created this PR for the bootstrap file. I'm having some challenges with tests + linting. The tests seem to be running with jinja2==2.11.3. The linting is running with jinja2==3.0.1. The linting fails because the test references, in a version specific way, a variable that stopped existing in 3.0.0. I get the error:
Is |
Head branch was pushed to by a user without write access
@owais @codeboten Could you give me advice on how to proceed? When I run The performance regression in the most recent run seems spurious (adding a space to a comment is probably unrelated...) |
@andrew-matteson |
@owais Nope. I'm also stumped about how to get past the generate check. |
|
@andrew-matteson please update @codeboten we use a specific GIT SHA in CI but use |
Yeah, I realized it right after posting the comment :) |
Head branch was pushed to by a user without write access
That should do it 🤞 |
The failures seem like sporadic issues unrelated to my changes. If I need to do something, let me know. I won't be able to touch this again until next Wednesday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will merge if everything else other than docker-tests passes which is known to be broken right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
The Jinja2 API hasn't changed the functions that have been wrapped in at least 8 years.
The version supported should be much more broad.
Specifically,
jinja2.environment.Template.render jinja2.Template.generate jinja2.Environment.compile jinja2.Environment._load_template
which are the only wrapped functions seem to be appropriate for the instrumentation wrappers without further changes.
I'm not a jinja2 expert, so it's possible I'm missing something. Happy to help with changes if it seems that I am.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
This change produces useful traces in our system.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.