Restoring metrics in requests by ashu658 · Pull Request #1110 · open-telemetry/opentelemetry-python-contrib

ashu658 · 2022-05-31T11:47:31Z

Description

Restoring production of metrics for requests library.

Fixes #1039

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Tested by sending metrics to collector and added unit test.

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
Documentation has been updated

linux-foundation-easycla · 2022-05-31T11:47:33Z

The committers listed above are authorized under a signed CLA.

✅ login: ashu658 / name: Ashutosh Goel (784bb02)

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py

CHANGELOG.md

srikanthccv · 2022-06-07T07:23:18Z

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py


+        metric_labels = {
+            "http.method": method,
+            "http.url": url,


Is this url parametrized or plain raw url? Otherwise, this can lead very to high cardinality.

This is the same url we add in span attributes. I think its not parameterised. (i.e. when added in metric it would look something like this http.url: STRING(https://www.example.com/123/?key1=value1)
I cannot think of a way to find encoded parameters from url to remove them. (e.g. in the above example remove 123 from path).
Open for suggestions :)

That's not a problem with tracing but here each url creates a new time series which should be avoided. If there is no way to get the parametrized url I would suggest remove this tag entirely.

Removed url tag. But there could be 2 different urls in a single time series (as there might be no differentiating tag). I am not sure if this is okay. Any suggestions?

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

ocelotl · 2022-06-07T13:46:35Z

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

+        }
+
+        try:
+            parsed_url = urlparse(url)


I imagine that the call to urlparse is what can cause the ValueError exception to be raised. If that happens, does it make sense to continue the instrumentation with a metrics_labels dictionary that will not have "http.host" and "http.scheme" keys?

In traces we add the url even if the url is invalid (e.g. url = random123 is also added as span attribute). So, I added it here for consistency.
I think its okay to go without http.host and http.scheme keys as http.url attribute alone is one of the recommended set of attributes for client metrics (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives)

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

lzchen · 2022-06-07T17:14:09Z

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py

+            "http.host": "httpbin.org",
+            "http.method": "GET",
+            "http.flavor": "1.1",
+            "http.scheme": "http",


Are we missing a few? net.*, http.target?

Added net.* attributes.
I have not added http.target as the target is not parameterised and adding raw target can lead to high cardinality.
Its similar for http.url as discussed here

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py

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

1) Using default_timer 2) Refactoring varibale names 3) Removing metric usage docstring

…/opentelemetry-python-contrib into restore-requests-metrics

srikanthccv · 2022-06-23T11:41:06Z

@ashu658 add _supports_metrics = True for requests similiar to this.

srikanthccv

LGTM, Thanks.

ashu658 requested a review from a team May 31, 2022 11:47

ashu658 changed the title ~~Restoring metrics in requests~~ [WIP] Restoring metrics in requests May 31, 2022

Restoring metrics in requests

784bb02

ashu658 force-pushed the restore-requests-metrics branch from d94f67e to 784bb02 Compare May 31, 2022 12:42

Updating CHANGELOG.md

feb34d8

ashu658 marked this pull request as draft May 31, 2022 13:03

Ashutosh Goel added 4 commits June 2, 2022 13:03

Fixing build errors

0d58aaa

Fixing unit tests

cc33431

Fixing unit tests

c5224fc

Merge branch 'main' into restore-requests-metrics

f39393d

ashu658 marked this pull request as ready for review June 6, 2022 07:11

Fixing lint errors

f497101

ashu658 changed the title ~~[WIP] Restoring metrics in requests~~ Restoring metrics in requests Jun 6, 2022

srikanthccv reviewed Jun 6, 2022

View reviewed changes

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py Outdated Show resolved Hide resolved

Using memory_metrics_reader in unit tests

4cca547

srikanthccv reviewed Jun 7, 2022

View reviewed changes

ocelotl reviewed Jun 7, 2022

View reviewed changes

lzchen reviewed Jun 7, 2022

View reviewed changes

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Jun 7, 2022

View reviewed changes

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Jun 7, 2022

View reviewed changes

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py Show resolved Hide resolved

lzchen reviewed Jun 7, 2022

View reviewed changes

Ashutosh Goel and others added 7 commits June 14, 2022 15:39

Merge branch 'main' into restore-requests-metrics

7f843cb

Changed docstring to remove errors

b872ada

Fixing lint errors

5c8cacb

Merge branch 'main' into restore-requests-metrics

68e34ee

Resolving comments: Removing url tag and adding new attributes

80bdff2

Merge branch 'main' into restore-requests-metrics

7db639b

Removing unwanted changes

89d8f6e

ocelotl reviewed Jun 21, 2022

View reviewed changes

instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py Outdated Show resolved Hide resolved

srikanthccv reviewed Jun 21, 2022

View reviewed changes

ashu658 and others added 6 commits June 23, 2022 00:17

Update instrumentation/opentelemetry-instrumentation-requests/tests/t…

b273409

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

Resolving comments:

52de6c3

1) Using default_timer 2) Refactoring varibale names 3) Removing metric usage docstring

Merge branch 'restore-requests-metrics' of https://github.com/ashu658…

661843b

…/opentelemetry-python-contrib into restore-requests-metrics

Removing extra linebreak from docstring

8a0e994

Merge branch 'main' into restore-requests-metrics

077f4be

Fixing lint error

7f4a49b

ashu658 self-assigned this Jun 23, 2022

Ashutosh Goel added 2 commits June 24, 2022 16:02

Adding _support_metrics variable

32dab15

Adding new line at the end of file

e57651b

srikanthccv approved these changes Jun 24, 2022

View reviewed changes

srikanthccv requested review from lzchen and ocelotl June 24, 2022 15:11

lzchen approved these changes Jun 24, 2022

View reviewed changes

srikanthccv merged commit f7fd1e0 into open-telemetry:main Jun 25, 2022

Conversation

ashu658 commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Uh oh!

linux-foundation-easycla bot commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srikanthccv Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

ashu658 Jun 14, 2022 • 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!

srikanthccv Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

ashu658 Jun 21, 2022 • 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!

Uh oh!

ocelotl Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

ashu658 Jun 14, 2022 • 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!

Uh oh!

Uh oh!

Uh oh!

lzchen Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

ashu658 Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srikanthccv commented Jun 23, 2022

Uh oh!

srikanthccv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ashu658 commented May 31, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 31, 2022 •

edited

Loading

ashu658 Jun 14, 2022 •

edited

Loading

ashu658 Jun 21, 2022 •

edited

Loading

ashu658 Jun 14, 2022 •

edited

Loading