[FLASK] added request and response hook #416

NickSulistio · 2021-04-06T02:10:32Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Tested using tox -e test-instrumentation-flask, wrote two new tests

Does This PR Require a Core Repo Change?

Yes. - Link to PR:
No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

linux-foundation-easycla · 2021-04-06T02:10:35Z

The committers are authorized under a signed CLA.

✅ Nick (a0cdcc2)

lzchen · 2021-04-06T15:14:06Z

@NickSulistio
I would mark this PR as a draft if it is not ready to review yet.

…y-python-contrib into flask_hooks Updating my branch

…emetry-python-contrib into flask_hooks Get latest version

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py

owais · 2021-04-09T03:01:37Z

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py

+
+        FlaskInstrumentor().instrument(otel_request_hook=request_hook, otel_response_hook=response_hook)
+        # pylint: disable=import-outside-toplevel,reimported,redefined-outer-name
+        from flask import Flask


isn't flask being imported globally above? why do we need this?

Hey Owais, so if I don't import Flask here, no span will be created. I'm not sure exactly why this is the case but when I removed that import line, the span was no longer being created.

I think you'll be able to solve it by creating your flask app instance after calling instrument(). So for each test case, first call .instument() and then create flask app instance.

Hey Owais, if I'm not mistaken, aren't I already calling .instrument() before I import Flask and create the app instance?

I don't see the app being created anywhere in this method. May be it is being created in the super-class in which case it would be created before since super().setUp() is being called before instrument.

Hey Owais, for this test, we're testing the hooks without the App being created. I'm not entirely sure how this is supposed to work, but previously there was a test for TestProgrammaticCustomSpanNameWithoutApp, which did the same. I believe that @lonewolf3739 may have a better understanding of what this is used for.

@NickSulistio Sorry for getting back late. I think If you do import from flask import Flask first and next call FlaskInstrumentor().instrument()then instrumentation doesn't happen because the way patching is done; I remember this was the reason but I may be wrong.

If this is a limitation with the instrumentation itself, I think we can live with it for now and try to improve the instrumentation later.

owais · 2021-04-09T03:02:13Z

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py

+            {"http.target": "/hello/123", "http.route": "/hello/<int:helloid>", "hook_attr":"hello world"}
+        )
+
+        self.client.get("/hello/123")


I would also test adding a header to the response and making sure it was seen here.

If you don't mind, could you elaborate on how to do that?

You'd need to update the post response hook and inject headers into the response there. Look at Flask or WSGI docs (which ever object is passed to the response hook). Then you'd capture the result from client.get() call here and verify the result has the same headers you set in the hook earlier.

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

CHANGELOG.md

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

Co-authored-by: Leighton Chen <lechen@microsoft.com>

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py

NickSulistio · 2021-04-19T18:35:17Z

@ocelotl Would you like to review this PR?

lzchen · 2021-04-27T00:18:45Z

@owais
Have your concerns been addressed?

owais · 2021-04-27T09:45:04Z

Couple of minor changes/additions suggested. Please also remove the commented out code.

ocelotl

Looks good 👍 Minor changes requested

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py

…lemetry/instrumentation/flask/__init__.py Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

…y-python-contrib into flask_hooks

…emetry-python-contrib into flask_hooks

This reverts commit cbc0995.

…/opentelemetry-python-contrib into flask_hooks" This reverts commit dafb554, reversing changes made to 422d67d.

…sulistio/opentelemetry-python-contrib into flask_hooks"" This reverts commit e91fb0b.

This reverts commit d25c675.

This reverts commit 34e7013.

…emetry-python-contrib into flask_hooks

added request and response hook - may be very buggy

a0cdcc2

NickSulistio requested review from a team, ocelotl and lzchen and removed request for a team April 6, 2021 02:10

NickSulistio marked this pull request as draft April 6, 2021 18:28

Merge branch 'main' into flask_hooks

53ddfcd

NickSulistio changed the title ~~added request and response hook - may be very buggy~~ [FLASK] added request and response hook Apr 8, 2021

NickSulistio added 3 commits April 8, 2021 00:10

added req/resp hooks and tests

c0ab6a5

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

ddb3077

…y-python-contrib into flask_hooks Updating my branch

Merge branch 'flask_hooks' of https://github.com/nicksulistio/opentel…

ea965d1

…emetry-python-contrib into flask_hooks Get latest version

NickSulistio marked this pull request as ready for review April 8, 2021 07:23

Merge branch 'main' into flask_hooks

74bc40c

owais reviewed Apr 9, 2021

View reviewed changes

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py Outdated Show resolved Hide resolved

owais reviewed Apr 9, 2021

View reviewed changes

lzchen reviewed Apr 9, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lzchen reviewed Apr 9, 2021

View reviewed changes

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py Outdated Show resolved Hide resolved

NickSulistio and others added 3 commits April 12, 2021 11:17

Update CHANGELOG.md

8a4f2a3

Co-authored-by: Leighton Chen <lechen@microsoft.com>

removed name callback

4721cdf

Merge branch 'main' into flask_hooks

79d04fe

lzchen reviewed Apr 14, 2021

View reviewed changes

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py Outdated Show resolved Hide resolved

Merge branch 'main' into flask_hooks

f975067

lzchen approved these changes Apr 15, 2021

View reviewed changes

Merge branch 'main' into flask_hooks

1605884

NickSulistio mentioned this pull request Apr 29, 2021

REQUEST: New membership for NickSulistio open-telemetry/community#721

Closed

6 tasks

added if callable check

2757171

lzchen approved these changes May 26, 2021

View reviewed changes

Merge branch 'main' into flask_hooks

327905e

ocelotl approved these changes May 27, 2021

View reviewed changes

...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py Outdated Show resolved Hide resolved

instrumentation/opentelemetry-instrumentation-flask/tests/test_programmatic.py Outdated Show resolved Hide resolved

NickSulistio and others added 2 commits May 27, 2021 13:12

Update instrumentation/opentelemetry-instrumentation-flask/src/opente…

6dd503c

…lemetry/instrumentation/flask/__init__.py Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

removed useless comment

2f2611e

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

srikanthccv approved these changes May 30, 2021

View reviewed changes

srikanthccv mentioned this pull request May 30, 2021

Allow supplying a callback for adding new attributes when spans start open-telemetry/opentelemetry-specification#1724

Open

lzchen and others added 19 commits June 1, 2021 08:54

Merge branch 'main' into flask_hooks

e941831

tried linting

34e7013

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

5534ed1

…y-python-contrib into flask_hooks

linted and merged

b867ead

Merge branch 'flask_hooks' of https://github.com/nicksulistio/opentel…

ddc10d7

…emetry-python-contrib into flask_hooks

linting and merge

cbc0995

removed venv

d4eab75

removed venv

422d67d

Merge branch 'flask_hooks' of https://github.com/nicksulistio/opentel…

dafb554

…emetry-python-contrib into flask_hooks

Revert "linting and merge"

d25c675

This reverts commit cbc0995.

Revert "Merge branch 'flask_hooks' of https://github.com/nicksulistio…

e91fb0b

…/opentelemetry-python-contrib into flask_hooks" This reverts commit dafb554, reversing changes made to 422d67d.

Revert "Revert "Merge branch 'flask_hooks' of https://github.com/nick…

aa37da5

…sulistio/opentelemetry-python-contrib into flask_hooks"" This reverts commit e91fb0b.

Revert "Revert "linting and merge""

7b017ec

This reverts commit d25c675.

reverted

add1367

Revert "tried linting"

a8f353e

This reverts commit 34e7013.

Revert "tried linting"

1aafe2b

This reverts commit 34e7013.

Merge branch 'flask_hooks' of https://github.com/nicksulistio/opentel…

e95313f

…emetry-python-contrib into flask_hooks

reverted to d4eab75

ff10d2b

fixed generate

52cfe53

lzchen merged commit fbd39cc into open-telemetry:main Jun 1, 2021

NickSulistio deleted the flask_hooks branch June 1, 2021 22:08

alexmojaki mentioned this pull request Feb 7, 2024

Span names for flask requests only show the HTTP method if they didn't match a route #2155

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLASK] added request and response hook #416

[FLASK] added request and response hook #416

NickSulistio commented Apr 6, 2021 •

edited

Loading

linux-foundation-easycla bot commented Apr 6, 2021 •

edited

Loading

lzchen commented Apr 6, 2021

owais Apr 9, 2021

NickSulistio Apr 12, 2021

owais Apr 27, 2021

NickSulistio Apr 27, 2021

owais Apr 27, 2021

NickSulistio Apr 30, 2021

srikanthccv May 6, 2021

owais May 7, 2021

owais Apr 9, 2021

NickSulistio Apr 12, 2021

owais Apr 27, 2021

NickSulistio commented Apr 19, 2021

lzchen commented Apr 27, 2021

owais commented Apr 27, 2021

ocelotl left a comment

[FLASK] added request and response hook #416

[FLASK] added request and response hook #416

Conversation

NickSulistio commented Apr 6, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

linux-foundation-easycla bot commented Apr 6, 2021 • edited Loading

lzchen commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickSulistio commented Apr 19, 2021

lzchen commented Apr 27, 2021

owais commented Apr 27, 2021

ocelotl left a comment

Choose a reason for hiding this comment

NickSulistio commented Apr 6, 2021 •

edited

Loading

linux-foundation-easycla bot commented Apr 6, 2021 •

edited

Loading