Skip to content
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

Add metric instrumentation for urllib #1553

Merged
merged 16 commits into from
Jan 15, 2023

Conversation

shalevr
Copy link
Member

@shalevr shalevr commented Dec 28, 2022

Description

Add metrics instrumentation for urllib accroding to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server

Fixes #1041

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • 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

  • tox -e test-instrumentation-urllib

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

@shalevr shalevr requested a review from a team December 28, 2022 10:46
key=lambda m: m.name,
)

def assert_metric_expected(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used utils function for metrics assert
they can be deleted after open-telemetry/opentelemetry-python#3092 merged

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some reservations about the PR on the main repo. I will leave a review there about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@shalevr shalevr force-pushed the Metric-instrumentation-Urllib branch 2 times, most recently from 2e0961f to a045675 Compare December 28, 2022 12:49
@shalevr shalevr force-pushed the Metric-instrumentation-Urllib branch from a045675 to 749994b Compare December 28, 2022 14:05
Copy link
Contributor

@avzis avzis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great

@@ -214,6 +225,10 @@ def _instrumented_open_call(
SpanAttributes.HTTP_FLAVOR
] = f"{ver_[:1]}.{ver_[:-1]}"

_record_histograms(
histograms, labels, request, result, elapsed_time
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be a case where the result is None, but we should record the timing regardless of its return value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right this line should be outside the condition

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it so if the result is None we add records only for http_client_duration and http_client_request size,
the status code and HTTP flavor are missing from attributes because we take them from the response.
how does it sound?

)

data = getattr(request, "data", None)
request_size = 0 if data is None else len(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be a compressed size? Does data always return an iterable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@shalevr shalevr Jan 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment, I checked this issue while I wrote the code and the difference is the urllib api works only with bytes, and urllib3 works with bytes and IO(like BytesIO)

)

response_size = int(response.headers.get("Content-Length", 0))
histograms[MetricInstruments.HTTP_CLIENT_RESPONSE_SIZE].record(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about the compression as the instrument description make it explicit

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

key=lambda m: m.name,
)

def assert_metric_expected(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some reservations about the PR on the main repo. I will leave a review there about it.

@srikanthccv srikanthccv merged commit c92ba14 into open-telemetry:main Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[instrumentation-urllib] Restore metrics
3 participants