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.

Sign up for GitHub

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

Jump to bottom

metric instrumentation Tornado #1252

Merged
merged 25 commits into from
Oct 12, 2022
Merged

metric instrumentation Tornado #1252

merged 25 commits into from
Oct 12, 2022

Conversation

shalevr
Copy link
Member

@shalevr shalevr commented Aug 30, 2022 •
edited by srikanthccv
Loading

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 #1145

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-tornado

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

@shalevr shalevr requested a review from a team August 30, 2022 11:39
@shalevr shalevr changed the title Add metrics instrumentation metrics instrumentation Tornado Aug 30, 2022
@shalevr shalevr changed the title metrics instrumentation Tornado metric instrumentation Tornado Aug 30, 2022
shalevr added 3 commits August 30, 2022 14:53
@shalevr
Add entry to CHANGELOG.md
4d52bbb
@shalevr
Merge branch 'main' into metrics-instrumentation-tornado
c336916 
* 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
@shalevr
Run tox generate
f5369ed
@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch from a2a7333 to 7e01f9e Compare August 30, 2022 12:50
@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch from 7e01f9e to df4dcf8 Compare August 30, 2022 14:22
@srikanthccv srikanthccv mentioned this pull request Sep 15, 2022
Closed
@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch from d2223d9 to 033c62c Compare September 22, 2022 08:57
@shalevr
Copy link
Member Author

shalevr commented Sep 22, 2022

I changed the test format to be more readable,
and I think to move the get_sorted_metrics, sorted_metrics, and assertMetricHasAttributes to TestBase
what do you think?

@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch from 44fa143 to 6dc5359 Compare October 2, 2022 11:56
@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch 4 times, most recently from efdd5e7 to 4859562 Compare October 3, 2022 15:43
@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch from 4859562 to 9f24abf Compare October 3, 2022 16:01
@shalevr shalevr requested a review from lzchen October 5, 2022 21:31
lzchen
lzchen reviewed Oct 11, 2022
View reviewed changes
.../opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py Outdated


def _record_prepare_metrics(server_histograms, handler):
request_size = len(handler.request.body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should be 0 if body is None.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

lzchen
lzchen reviewed Oct 11, 2022
View reviewed changes
.../opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py
_SERVER_REQUEST_SIZE_HISTOGRAM = "http.server.request.size"
_SERVER_RESPONSE_SIZE_HISTOGRAM = "http.server.response.size"
_SERVER_ACTIVE_REQUESTS_HISTOGRAM = "http.server.active_requests"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add the constants for client up here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

lzchen
lzchen reviewed Oct 11, 2022
View reviewed changes
.../opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py Outdated

server_histograms = _create_server_histograms(meter)

client_duration_histogram = meter.create_histogram(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can refactor to use _create_client_histograms as well to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

lzchen
lzchen approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM. Just a few non-blocking suggestions.

srikanthccv
srikanthccv reviewed Oct 12, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -74,6 +74,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#1197](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1197))
- Add metric instumentation for flask
([#1186](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1186))
- Add metric instrumentation for tornado
([#1252](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1252))
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to Unreleased section

Copy link
Member Author

@shalevr shalevr Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Like this?

Copy link
Member

Choose a reason for hiding this comment

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

This was part of prev release changelog prior. I mean add the entry under #Added of Unreleased section

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh I got it!
Fixed

@shalevr shalevr force-pushed the metrics-instrumentation-tornado branch from 3533659 to 6044ab2 Compare October 12, 2022 13:36
@shalevr shalevr requested a review from srikanthccv October 12, 2022 14:30
@srikanthccv srikanthccv merged commit 744851b into open-telemetry:main Oct 12, 2022
@shalevr shalevr mentioned this pull request Oct 13, 2022
Closed
6 tasks
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
@shalevr
metric instrumentation Tornado (open-telemetry#1252)
75332e2
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
@shalevr
metric instrumentation Tornado (open-telemetry#1252)
1fd3771
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
@shalevr
metric instrumentation Tornado (open-telemetry#1252)
bc03a37
@shalevr shalevr mentioned this pull request Dec 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@lzchen lzchen lzchen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metrics instrumentation tornado
3 participants
@shalevr @lzchen @srikanthccv