#1726 Update handler name used by SQLAlchemy
Merged 9 months ago by mikem. Opened 9 months ago by breilly.
breilly/fm-orchestrator dbapierror  into  master

@@ -15,6 +15,7 @@ 

      Histogram,

      start_http_server,

  )

+ import sqlalchemy

  from sqlalchemy import event

  

  
@@ -85,9 +86,11 @@ 

      if not target:

          target = db.engine

  

-     @event.listens_for(target, "dbapi_error", named=True)

-     def receive_dbapi_error(**kw):

-         db_dbapi_error_counter.inc()

+     if sqlalchemy.__version__[0] == "0":

+         # Deprecated since 0.9

+         @event.listens_for(target, "dbapi_error", named=True)

+         def receive_dbapi_error(**kw):

+             db_dbapi_error_counter.inc()

  

      @event.listens_for(target, "engine_connect")

      def receive_engine_connect(conn, branch):

rebased onto 17bd95f5c58e0f50667d15735c88a85e53f0f4ad

9 months ago

Build 17bd95f5c58e0f50667d15735c88a85e53f0f4ad FAILED!
Rebase or make new commits to rebuild.

Build 17bd95f5c58e0f50667d15735c88a85e53f0f4ad FAILED!
Rebase or make new commits to rebuild.

My initial reaction is that this is removing functionality. Seems like we should conditionally use the handle_error event for newer versions.

https://docs.sqlalchemy.org/en/13/core/events.html#sqlalchemy.events.ConnectionEvents.handle_error

It's got a bit of a different signature compared to db_dbapi_error (though that doesn't really matter since we're just incrementing a counter).
https://docs.sqlalchemy.org/en/13/core/events.html#sqlalchemy.events.ConnectionEvents.dbapi_error

Here is a similar solution from another project:
https://github.com/DataDog/dd-trace-py/pull/1324/files

Also I think the check is backwards. if sqlalchemy.__version__[0] != "0" is the newer lib case.

Unit tests fail in py2 unit tests.

tests/test_common/test_monitor.py::TestViews::test_metrics FAILED [ 22%]
tests/test_common/test_monitor.py::test_standalone_metrics_server FAILED

The failed assertions are simply metric counts, so I think with my suggested above this error will go away.

e.g.

self = <tests.test_common.test_monitor.TestViews instance at 0x7f560e5af488>, provide_test_data = None

    def test_metrics(self, provide_test_data):
        rv = self.client.get("/module-build-service/1/monitor/metrics")

        count = len([
            line for line in rv.get_data(as_text=True).splitlines()
            if (line.startswith("# TYPE") and "_created " not in line)
        ])
>       assert count == num_of_metrics
E       assert 17 == 18
E         -17
E         +18

tests/test_common/test_monitor.py:29: AssertionError

rebased onto 923c27075be76e9726af09c846ed3911b94a5f3c

9 months ago

Build 923c27075be76e9726af09c846ed3911b94a5f3c FAILED!
Rebase or make new commits to rebuild.

rebased onto 737a1c14fe3c68e8e4d1931a54b8c6127fdf8534

9 months ago

Also updated the num_of_metrics value since it's now going to either have handle_error or dbapi_error, not both.

Build 737a1c14fe3c68e8e4d1931a54b8c6127fdf8534 FAILED!
Rebase or make new commits to rebuild.

Ah, this is what I get for just looking at the diff. I didn't realize we were already grabbing handle_error.

I guess to properly preserve the counter, we'd need to analyze the error in handle_error and determine if it is a dbapi error. However, that can probably wait for now. We can file a separate issue as a reminder

I think the simplest approach is pretty close to your original version. Something like:
https://pagure.io/fork/mikem/fm-orchestrator/commits/pr1726updates

I think it's easier to just keep the counter around, even in situations where we're not incrementing it. When we reach the point where we can stop supporting rhel7, then we can just drop it entirely.

Side note: db_dbapi_error wasn't removed until 1.4 (see here), but probably ok to just stick with the simpler version check we have rather than adding a bunch of code to parse the version string.

rebased onto a4f34450b1ee38062f36119da5227f6f32551e04

9 months ago

rebased onto 1883fa8

9 months ago

Build 1883fa8 FAILED!
Rebase or make new commits to rebuild.

Build 1883fa8 FAILED!
Rebase or make new commits to rebuild.

Commit 889a209 fixes this pull-request

Pull-Request has been merged by mikem

9 months ago

Filed #1727 as a follow up for later

Metadata