Add jinja2 instrumentation by majorgreys · Pull Request #643 · open-telemetry/opentelemetry-python

majorgreys · 2020-05-04T04:38:22Z

Fixes #631

Oberon00 · 2020-05-04T11:05:20Z

ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__init__.py

+        try:
+            return wrapped(*args, **kwargs)
+        finally:
+            span.set_attribute("component", "template")


There is no component any longer in the semantic conventions.

Is there a link for the background on that change? This has been unclear to me when looking over the code.

This change was done in open-telemetry/opentelemetry-specification#447, see also open-telemetry/opentelemetry-specification#271. Basically, it was deemed redundant. Previously you could check for component == http to determine if it is an HTTP span, now you can still check for hasAttribute(http.method).

Oberon00 · 2020-05-04T11:07:14Z

ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__init__.py

+            return wrapped(*args, **kwargs)
+        finally:
+            span.set_attribute("component", "template")
+            span.set_attribute("jinja2.template_name", template_name)


Template engines are widely used across different languages. Have you considered bringing up a spec PR for semantic conventions for templating engines? I think we should aim for cross-language cross-framework consistency here. E.g. maybe templating.engine=jinja2, templating.template=FILENAME would be good candidates?

As long as we don't have such conventions, using jinja2 as prefix is however probably the most name-clash-safe way.

Also, please set these attributes already when creating the span, so that they are available to samplers.

Certainly agree about a spec PR and can start a conversation about that as a follow-up from this PR.

ext/opentelemetry-ext-jinja2/tests/conftest.py

mauriciovasquezbernal

Looks good overall.

Few comments about some details I found around.

ext/opentelemetry-ext-jinja2/README.rst

ext/opentelemetry-ext-jinja2/setup.cfg

ext/opentelemetry-ext-jinja2/setup.py

ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__init__.py

ext/opentelemetry-ext-jinja2/tests/conftest.py

ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__init__.py

ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

…it__.py Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal

LGTM, all the comments are simple nits.

I tested by using:

$ cat templates/base.html 
Message: {% block content %}{% endblock %}
$ cat templates/template.html 
{% extends 'base.html' %}
{% block content %}Hello {{name}}!{% endblock %}

from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import (
    SimpleExportSpanProcessor,
    ConsoleSpanExporter,
)

trace.set_tracer_provider(TracerProvider())
trace.get_tracer_provider().add_span_processor(
  SimpleExportSpanProcessor(ConsoleSpanExporter())
)


from jinja2 import Environment, FileSystemLoader
from opentelemetry.ext.jinja2 import Jinja2Instrumentor

Jinja2Instrumentor().instrument()

env = Environment(loader=FileSystemLoader("templates"))
template = env.get_template('template.html')

I see how two spans are generated.

ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__init__.py

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

ext/opentelemetry-ext-jinja2/setup.cfg

mauriciovasquezbernal

LGTM again. Thanks for handling all my comments.

toumorokoshi

A couple small comments, but otherwise LGTM!

ext/opentelemetry-ext-jinja2/setup.cfg

toumorokoshi · 2020-05-11T05:27:03Z

ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__init__.py

+                )
+
+
+def _unwrap(obj, attr):


an aside, but unwrapping looks to be everywhere now. We might want to see what we can do to consolidate: #667

Agreed, will comment on the issue and we can refactor the unwrapping code accordingly.

ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

Migrating the jinja2 plugin forked from ddtracepy. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

* feat(plugin-http): add/modify attributes closes open-telemetry#373, open-telemetry#394 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca> * fix: change remotePort to localPort refactor: remove useless checks test: add assertions Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca> * test(plugin-https): sync with http plugin Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca> Co-authored-by: Mayur Kale <mayurkale@google.com>

majorgreys requested a review from a team May 4, 2020 04:38

majorgreys force-pushed the majorgreys/jinja2 branch 2 times, most recently from 4cac1a8 to 263f6aa Compare May 4, 2020 04:42

feat: add jinja2 instrumentation

cf8c885

majorgreys force-pushed the majorgreys/jinja2 branch from 263f6aa to cf8c885 Compare May 4, 2020 04:43

Oberon00 reviewed May 4, 2020

View reviewed changes

majorgreys added 2 commits May 4, 2020 23:51

fix attributes

5b604dc

remove unused

8f6790e

majorgreys force-pushed the majorgreys/jinja2 branch from 4cb1e71 to 8f6790e Compare May 5, 2020 04:00

docs

7b1d498

majorgreys force-pushed the majorgreys/jinja2 branch from 58ad0a7 to 7b1d498 Compare May 5, 2020 13:38

codeboten reviewed May 5, 2020

View reviewed changes

ext/opentelemetry-ext-jinja2/tests/conftest.py Outdated Show resolved Hide resolved

mauriciovasquezbernal reviewed May 5, 2020

View reviewed changes

majorgreys and others added 11 commits May 7, 2020 11:45

Update ext/opentelemetry-ext-jinja2/README.rst

fb3cbf9

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/setup.cfg

d343254

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__in…

70a2d84

…it__.py Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

move entrypoint

2e0cdcb

remove conftest.py

8c609a0

pass tracer to wrapped

aa43501

fix doc

752c959

add parent-child tests

86b60e2

Update ext/opentelemetry-ext-jinja2/src/opentelemetry/ext/jinja2/__in…

485e8c1

…it__.py Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

add test for uninstrumented

df5b207

fix typo

490800f

mauriciovasquezbernal approved these changes May 7, 2020

View reviewed changes

majorgreys and others added 5 commits May 8, 2020 08:30

use instrumentor as singleton

3ae9764

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

d7a675e

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

aa29c9c

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

1bd7c4c

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

546c8ba

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

majorgreys and others added 7 commits May 8, 2020 08:31

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

6cfb17b

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

2f0fd4f

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

Update ext/opentelemetry-ext-jinja2/tests/test_jinja2.py

f154820

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

call tearDown

db44456

make decorator private

ed17890

Merge remote-tracking branch 'upstream/master' into majorgreys/jinja2

c87933e

remove unnecessary doc

6392bb5

majorgreys requested review from Oberon00, codeboten and mauriciovasquezbernal May 8, 2020 12:44

codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 8, 2020

mauriciovasquezbernal reviewed May 8, 2020

View reviewed changes

ext/opentelemetry-ext-jinja2/setup.cfg Outdated Show resolved Hide resolved

mauriciovasquezbernal approved these changes May 8, 2020

View reviewed changes

toumorokoshi approved these changes May 11, 2020

View reviewed changes

majorgreys added 2 commits May 11, 2020 07:49

feedback

4353a8d

Merge remote-tracking branch 'upstream/master' into majorgreys/jinja2

0c2c269

majorgreys force-pushed the majorgreys/jinja2 branch from a30ac27 to 0c2c269 Compare May 11, 2020 12:17

toumorokoshi merged commit 1140332 into open-telemetry:master May 11, 2020

majorgreys deleted the majorgreys/jinja2 branch May 11, 2020 18:25

owais pushed a commit to owais/opentelemetry-python that referenced this pull request May 22, 2020

jinja2: Add jinja2 instrumentation (open-telemetry#643)

2863846

Migrating the jinja2 plugin forked from ddtracepy. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

majorgreys mentioned this pull request Jun 8, 2020

REQUEST: New membership for majorgreys open-telemetry/community#378

Closed

6 tasks

Conversation

majorgreys commented May 4, 2020

Uh oh!

Oberon00 May 4, 2020

Choose a reason for hiding this comment

Uh oh!

majorgreys May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Oberon00 May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oberon00 May 4, 2020

Choose a reason for hiding this comment

Uh oh!

majorgreys May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toumorokoshi May 11, 2020

Choose a reason for hiding this comment

Uh oh!

majorgreys May 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Oberon00 May 5, 2020 •

edited

Loading