Metrics instrumentation flask #1186

TheAnshul756 · 2022-07-07T10:54:40Z

Description

Fixes #1155

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

tox -e test-instrumentation-flask

Does This PR Require a Core Repo Change?

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

linux-foundation-easycla · 2022-07-07T10:54:44Z

The committers listed above are authorized under a signed CLA.

✅ login: TheAnshul756 / name: Anshul Asawa (e91b396, 139a1cc, 02d7f29)

ocelotl · 2022-07-07T13:01:31Z

@TheAnshul756 is this PR ready for review?

If yes, please remove the [WIP] from its name.
If not, please convert it to draft.

srikanthccv · 2022-07-11T19:47:08Z

Good part of this is same as in WSGI instrumentation. I am wondering if we can base all wsgi compatible frameworks on it.

TheAnshul756 · 2022-07-12T06:24:58Z

Yes I think we can implement all the wsgi based frameworks taking inspiration from WSGI Instrumentation but I don't think we can have a common function to do this job in all frameworks. Correct me if I misunderstood what you are saying.

srikanthccv · 2022-07-12T06:46:47Z

Many of WSGI complaint instrumentations (flask, falcon, pyramid, etc..) already take dep on opentelemetry-instrumentation-wsgi and use it.

srikanthccv · 2022-07-12T06:51:32Z

And when both WSGI and instrumentation are enabled they produce idetical metrics with very little changes in the time series.

TheAnshul756 · 2022-07-12T08:26:13Z

If in case we are only using specific framework instrumentation and not wsgi instrumentation then we have to have metric instrumentation implemented in the that framework, because although the frameworks take dependency on wsgi but they do not use it for metric or trace instrumentation. They have their own implementation.

srikanthccv · 2022-07-12T09:52:20Z

because although the frameworks take dependency on wsgi but they do not use it for metric or trace instrumentation.

What do you mean? Why else do you think we are taking dep on wsgi instrumentation. Because we use parts of it.

They have their own implementation.

Right, I am not saying they will not have any implementation on their own. But I don't see the reason each of them maintaining their own HTTP server semantic attributes, parsing the request response data from wsgi environ and filtering based on values and testing exact same thing unless you add some new tests (which is not the case with this PR).

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

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

lzchen · 2022-07-13T20:28:00Z

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

@@ -290,7 +307,21 @@ def _teardown_request(exc):
            # a way that doesn't run `before_request`, like when it is created
            # with `app.test_request_context`.
            return
+        start = flask.request.environ.get(_ENVIRON_STARTTIME_KEY)
+        duration = max(round((default_timer() - start) * 1000), 0)


Shouldn't all this logic be in _start_response?

yes, that is also a possible way. But we are starting span in before_request and ending it in teardown_request so I did the same for metrics too. Also here it says that, it is discouraged, I'm also not sure what to prefer. If doing it in start_response is more acceptable then I can make the changes.

Yes, please do. That comment is relevant to only tracing API and they are referring to update_name call on span once it is started. It is discouraged because sampling decision would have already been made based on it and such edge cases.

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

srikanthccv · 2022-07-14T10:58:29Z

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

@@ -290,7 +307,21 @@ def _teardown_request(exc):
            # a way that doesn't run `before_request`, like when it is created
            # with `app.test_request_context`.
            return
+        start = flask.request.environ.get(_ENVIRON_STARTTIME_KEY)
+        duration = max(round((default_timer() - start) * 1000), 0)


Yes, please do. That comment is relevant to only tracing API and they are referring to update_name call on span once it is started. It is discouraged because sampling decision would have already been made based on it and such edge cases.

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

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

srikanthccv · 2022-07-14T11:05:47Z

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

@@ -326,14 +326,30 @@ def collect_custom_response_headers_attributes(response_headers):
    return attributes


-def _parse_status_code(resp_status):
+def parse_status_code(resp_status):


I would still keep this and other functions private. They are not intended to be user accessible. If there is really need we can always change from private to public but not vice versa.

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

srikanthccv

blocking to prevent accidental merge to make sure tests are updated

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

srikanthccv

Does uninstrument stop producing the measurements? Please add some tests for instruement/uninstrument and other HTTP verbs.

TheAnshul756 · 2022-08-01T11:31:11Z

Does uninstrument stop producing the measurements? Please add some tests for instruement/uninstrument and other HTTP verbs.

@srikanthccv I've added test for uninstrument.

What do you mean by other HTTP verbs?

srikanthccv · 2022-08-01T12:01:14Z

I meant something other than get. Could be something like client.{post,delete} etc...

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

TheAnshul756 · 2022-08-01T12:22:09Z

I meant something other than get. Could be something like client.{post,delete} etc...

I've changed get to post in one of the test.

I'm just curious why it was needed? Because I think instrumentation logic is written independent of the type of request so if it is working fine with get then it should work fine with post too? Correct me if I'm wrong.

srikanthccv · 2022-08-01T13:37:45Z

What I really wanted is a something like post request that sends form data and see the duration results.

lzchen · 2022-08-02T17:49:12Z

Please update the metrics support for this instrumentation here

srikanthccv · 2022-08-04T13:15:16Z

Or add _supports_metrics = True in package.py and run tox -e generate.

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

…hub.com/shalevr/opentelemetry-python-contrib into feature/Metrics-instrumentation-urllib3 * 'feature/Metrics-instrumentation-urllib3' of https://github.com/shalevr/opentelemetry-python-contrib: Metric instrumentation asgi (open-telemetry#1197) Metrics instrumentation flask (open-telemetry#1186) Adding sqlalchemy native tags in sqlalchemy commenter (open-telemetry#1206)

* main: Codespell ci (open-telemetry#1237) aiohttp-client: Fix producing additional spans with each newly created ClientSession (open-telemetry#1246) Remove support for 3.6 (open-telemetry#853) Added the Licence and Manifest file Restore metrics in django (open-telemetry#1208) fix typo in example codes (open-telemetry#1240) boto3sqs: Make propagation compatible with other instrumentations and add 'messaging.url' span attribute (open-telemetry#1234) Release 1.12.0-0.33b0 (open-telemetry#1223) Fix Flask instrumentation doc link (open-telemetry#1216) Feature/metrics instrumentation urllib3 (open-telemetry#1198) Metric instrumentation asgi (open-telemetry#1197) Metrics instrumentation flask (open-telemetry#1186) Adding sqlalchemy native tags in sqlalchemy commenter (open-telemetry#1206) ci: fix docs workflow failure (open-telemetry#1211) Add psycopg2 native tags to sqlcommenter (open-telemetry#1203) SQLCommenter semicolon bug fix (open-telemetry#1200) Sync with sdk setup from setUpClass to setUp (open-telemetry#1193) # Conflicts: # CHANGELOG.md # instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py # instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py

TheAnshul756 and others added 3 commits July 5, 2022 14:17

add metric instrumentation

e91b396

add metrics tests

139a1cc

Merge branch 'open-telemetry:main' into metrics-instrumentation-flask

02d7f29

TheAnshul756 requested a review from a team July 7, 2022 10:54

TheAnshul756 marked this pull request as draft July 8, 2022 06:34

TheAnshul756 and others added 2 commits July 8, 2022 12:32

add changlog and fix lint

528efb0

Merge branch 'main' into metrics-instrumentation-flask

8278b7a

TheAnshul756 marked this pull request as ready for review July 8, 2022 09:08

TheAnshul756 changed the title ~~Metrics instrumentation flask [WIP]~~ Metrics instrumentation flask Jul 8, 2022

Merge branch 'main' into metrics-instrumentation-flask

615bf12

TheAnshul756 added 3 commits July 13, 2022 16:22

use wsgi functions instaed of flask's own

aea88d4

Merge branch 'metrics-instrumentation-flask' of https://github.com/Th…

754e062

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

fix lint

cade131

lzchen reviewed Jul 13, 2022

View reviewed changes

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

lzchen reviewed Jul 13, 2022

View reviewed changes

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

lzchen reviewed Jul 13, 2022

View reviewed changes

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

lzchen reviewed Jul 13, 2022

View reviewed changes

change starttime and duration time

d6026f8

sanketmehta28 requested changes Jul 14, 2022

View reviewed changes

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Outdated Show resolved Hide resolved

...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Outdated Show resolved Hide resolved

srikanthccv reviewed Jul 14, 2022

View reviewed changes

sanketmehta28 approved these changes Jul 14, 2022

View reviewed changes

fix merge conflict

41ab66f

Merge branch 'main' into metrics-instrumentation-flask

22b11a5

srikanthccv suggested changes Jul 26, 2022

View reviewed changes

TheAnshul756 added 4 commits July 27, 2022 16:25

add duration test

9286231

Merge branch 'metrics-instrumentation-flask' of https://github.com/Th…

a758eab

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

fix lint

1a33b21

add attributes value test for metrics

dda7548

lzchen approved these changes Jul 27, 2022

View reviewed changes

remove unwanted import

9557035

srikanthccv reviewed Jul 28, 2022

View reviewed changes

add test for uninstrument

27cdbc7

Merge branch 'main' into metrics-instrumentation-flask

536bf25

TheAnshul756 added 2 commits August 1, 2022 17:41

change request type to post for testing

bc89aa0

Merge branch 'metrics-instrumentation-flask' of https://github.com/Th…

ab2bed1

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

add value test for post request

0ba053f

srikanthccv added 2 commits August 4, 2022 16:37

Merge branch 'main' into metrics-instrumentation-flask

9fe8a18

Merge branch 'main' into metrics-instrumentation-flask

3516ec4

TheAnshul756 added 2 commits August 5, 2022 12:11

add readme for metrics

f45ca51

Merge branch 'metrics-instrumentation-flask' of https://github.com/Th…

7cc4a50

…eAnshul756/opentelemetry-python-contrib into metrics-instrumentation-flask

srikanthccv approved these changes Aug 5, 2022

View reviewed changes

srikanthccv enabled auto-merge (squash) August 5, 2022 07:34

srikanthccv merged commit ebe6d18 into open-telemetry:main Aug 5, 2022

TheAnshul756 mentioned this pull request Oct 4, 2022

REQUEST: New membership for @TheAnshul756 open-telemetry/community#1226

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics instrumentation flask #1186

Metrics instrumentation flask #1186

TheAnshul756 commented Jul 7, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 7, 2022 •

edited

Loading

ocelotl commented Jul 7, 2022 •

edited

Loading

srikanthccv commented Jul 11, 2022

TheAnshul756 commented Jul 12, 2022

srikanthccv commented Jul 12, 2022

srikanthccv commented Jul 12, 2022

TheAnshul756 commented Jul 12, 2022

srikanthccv commented Jul 12, 2022

lzchen Jul 13, 2022

TheAnshul756 Jul 14, 2022

srikanthccv Jul 14, 2022

TheAnshul756 Jul 18, 2022

srikanthccv Jul 14, 2022

srikanthccv Jul 14, 2022

srikanthccv left a comment

srikanthccv left a comment

TheAnshul756 commented Aug 1, 2022

srikanthccv commented Aug 1, 2022

TheAnshul756 commented Aug 1, 2022

srikanthccv commented Aug 1, 2022

lzchen commented Aug 2, 2022

srikanthccv commented Aug 4, 2022

Metrics instrumentation flask #1186

Metrics instrumentation flask #1186

Conversation

TheAnshul756 commented Jul 7, 2022 • 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 Jul 7, 2022 • edited Loading

ocelotl commented Jul 7, 2022 • edited Loading

srikanthccv commented Jul 11, 2022

TheAnshul756 commented Jul 12, 2022

srikanthccv commented Jul 12, 2022

srikanthccv commented Jul 12, 2022

TheAnshul756 commented Jul 12, 2022

srikanthccv commented Jul 12, 2022

lzchen Jul 13, 2022

Choose a reason for hiding this comment

TheAnshul756 Jul 14, 2022

Choose a reason for hiding this comment

srikanthccv Jul 14, 2022

Choose a reason for hiding this comment

TheAnshul756 Jul 18, 2022

Choose a reason for hiding this comment

srikanthccv Jul 14, 2022

Choose a reason for hiding this comment

srikanthccv Jul 14, 2022

Choose a reason for hiding this comment

srikanthccv left a comment

Choose a reason for hiding this comment

srikanthccv left a comment

Choose a reason for hiding this comment

TheAnshul756 commented Aug 1, 2022

srikanthccv commented Aug 1, 2022

TheAnshul756 commented Aug 1, 2022

srikanthccv commented Aug 1, 2022

lzchen commented Aug 2, 2022

srikanthccv commented Aug 4, 2022

TheAnshul756 commented Jul 7, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 7, 2022 •

edited

Loading

ocelotl commented Jul 7, 2022 •

edited

Loading