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

Add metrics instrumentation sqlalchemy #1645

Merged
merged 27 commits into from
Feb 26, 2023
Merged

Add metrics instrumentation sqlalchemy #1645

merged 27 commits into from
Feb 26, 2023

Conversation

shalevr
Copy link
Member

@shalevr shalevr commented Feb 6, 2023 •
edited
Loading

Description

Add metrics instrumentation for sqlalchemy

Fixes #1143

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-sqlalchemy11
  • tox -e test-instrumentation-sqlalchemy14

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 as a code owner February 6, 2023 15:27
@shalevr shalevr marked this pull request as draft February 6, 2023 15:28
shalevr
shalevr commented Feb 6, 2023
View reviewed changes
...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py
self._register_event_listener(engine, "checkout", self._pool_checkout)

def _get_pool_name(self):
return self.engine.pool.logging_name or ""
Copy link
Member Author

Choose a reason for hiding this comment

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

The specification didn't write what to do in case the pool_name is not provided, so I kept it empty in this case.
There is a discussion about it in the specification, I'm waiting to see what will be decided

@shalevr shalevr force-pushed the Metrics-instrumentation-sqlalchemy branch from 51746e6 to 6e84d30 Compare February 6, 2023 17:08
@shalevr shalevr force-pushed the Metrics-instrumentation-sqlalchemy branch from 6e84d30 to ae56830 Compare February 6, 2023 17:25
@shalevr shalevr force-pushed the Metrics-instrumentation-sqlalchemy branch from e97f3c5 to eb5e1dc Compare February 6, 2023 19:30
@shalevr shalevr marked this pull request as ready for review February 6, 2023 21:44
@shalevr shalevr force-pushed the Metrics-instrumentation-sqlalchemy branch from 061be49 to b22f06a Compare February 7, 2023 11:50
nemoshlag
nemoshlag reviewed Feb 8, 2023
View reviewed changes
...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py
@@ -136,32 +139,47 @@ def _instrument(self, **kwargs):
An instrumented engine if passed in as an argument or list of instrumented engines, None otherwise.
"""
tracer_provider = kwargs.get("tracer_provider")
tracer = get_tracer(__name__, __version__, tracer_provider)
Copy link
Member

Choose a reason for hiding this comment

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

nice!

nemoshlag
nemoshlag reviewed Feb 8, 2023
View reviewed changes
instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy_metrics.py

def test_metrics_one_connection(self):
pool_name = "pool_test_name"
engine = sqlalchemy.create_engine(
Copy link
Member

Choose a reason for hiding this comment

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

consider moving the basic engine version to test setUp

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 thought about this, but the create_engine params are not the same for all of the tests

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of having the basic version as setUp and test-specific changes per test but this is not a blocker by any means

shalevr
shalevr commented Feb 8, 2023
View reviewed changes
.github/workflows/test.yml Outdated
@@ -6,7 +6,7 @@ on:
- 'release/*'
pull_request:
env:
CORE_REPO_SHA: 042d7a7921e25936decd95addf4fb1d30afb88e2
CORE_REPO_SHA: f5fb6b1353929cf8039b1d38f97450866357d901
Copy link
Member Author

Choose a reason for hiding this comment

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

Reference to this change:
open-telemetry/opentelemetry-python#3115

@shalevr shalevr mentioned this pull request Feb 16, 2023
Merged
10 tasks
@shalevr
Update test.yml
176e8e6 
merge with main
srikanthccv
srikanthccv reviewed Feb 18, 2023
View reviewed changes
CHANGELOG.md Outdated
@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- Add metrics instrumentation for sqlalchemy
Copy link
Member

@srikanthccv srikanthccv Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should go in ## Unreleased >> ### Added 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.

Fixed, Thank you

srikanthccv
srikanthccv reviewed Feb 18, 2023
View reviewed changes
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.

Did a firstpass review. Is it just the connection usage instrument that should be supported? Are there any other metrics expected?

@shalevr
Copy link
Member Author

shalevr commented Feb 18, 2023 •
edited
Loading

Did a firstpass review. Is it just the connection usage instrument that should be supported? Are there any other metrics expected?

The metrics instrumentation is implemented the same way for JS-contrib
open-telemetry/opentelemetry-js-contrib#1220

@srikanthccv
Copy link
Member

Looks like that's what defined in the spec as well so it should be fine.

shalevr
shalevr commented Feb 23, 2023
View reviewed changes
.github/workflows/test.yml Outdated
@@ -6,7 +6,7 @@ on:
- 'release/*'
pull_request:
env:
CORE_REPO_SHA: d0bb12b34b0c487198c935001636b6163485a50f
CORE_REPO_SHA: 2d1f0b9f5fce62549d1338882f37b91b95881c75
Copy link
Member Author

Choose a reason for hiding this comment

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

open-telemetry/opentelemetry-python#3182

@shalevr
Copy link
Member Author

shalevr commented Feb 23, 2023 •
edited
Loading

Got some problems with the old functions. I am working on it in a separate PR

@shalevr
Copy link
Member Author

shalevr commented Feb 23, 2023

Got some problems with the old functions. I am working on it in a separate PR

#1688

@srikanthccv srikanthccv merged commit 7ffbfc3 into open-telemetry:main Feb 26, 2023
@shalevr shalevr mentioned this pull request Feb 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv left review comments

@nemoshlag nemoshlag nemoshlag 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 sqlalchemy
4 participants
@shalevr @srikanthccv @nemoshlag @lzchen