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 the logfire.instrument_mysql() #341

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

aditkumar72
Copy link
Contributor

Fixes #310

Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b185a55) to head (616837a).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #341   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          119       121    +2     
  Lines         8895      8947   +52     
  Branches      1159      1162    +3     
=========================================
+ Hits          8895      8947   +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aditkumar72
Copy link
Contributor Author

@Kludex Could you please review the PR ?

@Kludex Kludex changed the title Add the logfire.instrument_mysql() Add the logfire.instrument_mysql() Jul 29, 2024
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

On this integration, you can also instrument a single connection.

@alexmojaki Can you confirm we want this to be analogous to psycopg?

@alexmojaki
Copy link
Contributor

We do want that, but I don't know if that needs to block this PR.

@aditkumar72
Copy link
Contributor Author

aditkumar72 commented Jul 29, 2024

@Kludex @alexmojaki I will update this PR instrumenting single MySQL connection, as this is not an urgent required changes

@aditkumar72 aditkumar72 force-pushed the mysql-instrument branch 2 times, most recently from 9363b75 to f62646d Compare July 31, 2024 08:08
@aditkumar72
Copy link
Contributor Author

@Kludex updated the PR, Could you please review ?

logfire/__init__.py Show resolved Hide resolved
logfire/_internal/main.py Outdated Show resolved Hide resolved
logfire/_internal/main.py Outdated Show resolved Hide resolved
tests/otel_integrations/test_mysql.py Outdated Show resolved Hide resolved
@aditkumar72 aditkumar72 force-pushed the mysql-instrument branch 6 times, most recently from f2a8098 to e3449ee Compare August 1, 2024 07:05
@aditkumar72
Copy link
Contributor Author

Hi @alexmojaki , resolved the comments. Could you please review ?

pyproject.toml Outdated Show resolved Hide resolved
tests/otel_integrations/test_mysql.py Outdated Show resolved Hide resolved
@aditkumar72
Copy link
Contributor Author

@alexmojaki Could you please review the changes ?

@aditkumar72 aditkumar72 requested a review from alexmojaki August 1, 2024 14:41
Comment on lines +1238 to +1244
Args:
conn: The `mysql` connection to instrument, or `None` to instrument all connections.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.

Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Args:
conn: The `mysql` connection to instrument, or `None` to instrument all connections.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.
Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine. We have something similar in instrument_psycopg. All of this is useful, especially the fact that it returns an instrumented connection.

Comment on lines +25 to +32

Args:
conn: The MySQL connection to instrument. If None, the entire `mysql` module is instrumented.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.

Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Args:
conn: The MySQL connection to instrument. If None, the entire `mysql` module is instrumented.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.
Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Comment on lines +629 to +636

Args:
conn: The `mysql` connection to instrument, or `None` to instrument all connections.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.

Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Args:
conn: The `mysql` connection to instrument, or `None` to instrument all connections.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.
Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Comment on lines +25 to +32

Args:
conn: The MySQL connection to instrument. If None, the entire `mysql` module is instrumented.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.

Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Args:
conn: The MySQL connection to instrument. If None, the entire `mysql` module is instrumented.
**kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` methods.
Returns:
If a connection is provided, returns the instrumented connection. If no connection is provided, returns None.

@aditkumar72
Copy link
Contributor Author

aditkumar72 commented Aug 2, 2024

@Kludex please check this comment #341 (comment)
Do we need to remove those doc strings ?

@aditkumar72 aditkumar72 requested a review from Kludex August 2, 2024 12:27
@Kludex Kludex merged commit 3fbea2f into pydantic:main Aug 5, 2024
13 checks passed
@Kludex
Copy link
Member

Kludex commented Aug 5, 2024

Thanks @aditkumar72 🙏

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.

Add the logfire.instrument_mysql()
3 participants