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 span for connection phase #1134

Merged

Conversation

shahargl
Copy link
Contributor

@shahargl shahargl commented Jun 15, 2022

Description

I've added the code that adds #1133

Fixes #1133
#1133

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

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

@shahargl shahargl requested a review from a team June 15, 2022 09:14
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 15, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! We need at least one test case that validates these changes are correct 🙂

@shahargl
Copy link
Contributor Author

shahargl commented Jun 16, 2022

@ocelotl no problem!

We need at least one test case that validates these changes are correct

Can you guide me through an example? You mean UT?

Also, I saw that some tests are failing now so I'll fix it

@shahargl
Copy link
Contributor Author

@ocelotl Fixed all tests and they pass now. Also - they're covering the connect phase, as every test use engine.connect (you can see the number of spans is now 2 instead of 1)

@shahargl shahargl requested a review from ocelotl June 16, 2022 14:04
@shahargl
Copy link
Contributor Author

@ocelotl
Added also a test for connection phase

@srikanthccv
Copy link
Member

you probably wanted to add listen on connect similar to this

listen(
engine, "before_cursor_execute", self._before_cur_exec, retval=True
)
listen(engine, "after_cursor_execute", _after_cur_exec)
listen(engine, "handle_error", _handle_error)

@shahargl
Copy link
Contributor Author

shahargl commented Jun 16, 2022

@srikanthccv
Hey! Thanks for jumping in :)

So - initially, I thought about this approach too. Still, the problem is that if I add a listener to the connect event, we will have only the time the connection is initialized. Without having the "connection completed" event, we won't be able to "finish" the span.

Unfortunately, sqlalchemy doesn't have this event (https://docs.sqlalchemy.org/en/14/core/events.html)
So I chose to patch the connect function so the full connection time will be traced. wdyt?

@srikanthccv
Copy link
Member

@shahargl makes sense

@shahargl
Copy link
Contributor Author

@ocelotl updated the code to allow SQLAlchemy 1.1 too

@shahargl shahargl requested a review from srikanthccv June 22, 2022 06:45
…pentelemetry/instrumentation/sqlalchemy/engine.py

Co-authored-by: Srikanth Chekuri <[email protected]>
@shahargl shahargl requested a review from srikanthccv June 27, 2022 10:17
@shahargl
Copy link
Contributor Author

@ocelotl @srikanthccv - fixed the tests :) feel free to approve/merge

@ocelotl
Copy link
Contributor

ocelotl commented Jul 4, 2022

@ocelotl @srikanthccv - fixed the tests :) feel free to approve/merge

I see some tests failing, please take a look, @shahargl

@shahargl
Copy link
Contributor Author

@ocelotl fixed, can you report the status so the tests will run?

dev-requirements.txt Outdated Show resolved Hide resolved
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. Minor comments.

CHANGELOG.md Outdated Show resolved Hide resolved
@shahargl
Copy link
Contributor Author

@ocelotl
Thanks again, fixed your comments. Sorry it takes too long, can you rerun the tests?

@shahargl
Copy link
Contributor Author

@ocelotl
4 tests failing with

17 | #include "codecs.h"
          |          ^~~~~~~~~~
    compilation terminated.
    error: command 'gcc' failed with exit status 1

So idk why, do you have idea?

Also, fixed the docker-tests

tox.ini Outdated Show resolved Hide resolved
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.

Instrument SQLAlchemy engine connection phase
3 participants