Merge lp:~lifeless/storm/timelinetracer into lp:storm

Proposed by Robert Collins on 2011-09-13
Status: Merged
Merged at revision: 402
Proposed branch: lp:~lifeless/storm/timelinetracer
Merge into: lp:storm
Diff against target: 214 lines (+77/-40)
2 files modified
storm/tracer.py (+29/-13)
tests/tracer.py (+48/-27)
To merge this branch: bzr merge lp:~lifeless/storm/timelinetracer
Reviewer Review Type Date Requested Status
James Henstridge 2011-09-13 Approve on 2011-09-13
Review via email: mp+75109@code.launchpad.net

Description of the change

To post a comment you must log in.
James Henstridge (jamesh) wrote :

Looks good. I noticed a few small issues, but it looks pretty much ready to merge:

[1]
+ # be preserved as that is where we are substiting values in.

Should be "substituting"

[2]
+ #param timeline_factory: A factory function to produce the timeline to

Should be "@param"

[3]
It looks like all the tests are running with conn.param_mark == '?'. It'd be good to have at least one that runs with a StubConnection with param_mark set to '%s' to test the other branch in connection_raw_execute.

review: Approve
Robert Collins (lifeless) wrote :

all actioned, pushing it up now. Thanks!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storm/tracer.py'
2--- storm/tracer.py 2011-09-12 06:43:04 +0000
3+++ storm/tracer.py 2011-09-13 04:08:34 +0000
4@@ -118,12 +118,20 @@
5 # There are some bind parameters so we want to insert them into
6 # the sql statement so we can log the statement.
7 query_params = list(connection.to_database(params))
8- # We need to ensure % symbols used for LIKE statements etc are
9- # properly quoted or else the string format operation will fail.
10- quoted_statement = re.sub(
11+ if connection.param_mark == '%s':
12+ # Double the %'s in the string so that python string formatting
13+ # can restore them to the correct number. Note that %s needs to
14+ # be preserved as that is where we are substiting values in.
15+ quoted_statement = re.sub(
16 "%%%", "%%%%", re.sub("%([^s])", r"%%\1", statement))
17- quoted_statement = convert_param_marks(
18- quoted_statement, connection.param_mark, "%s")
19+ else:
20+ # Double all the %'s in the statement so that python string
21+ # formatting can restore them to the correct number. Any %s in
22+ # the string should be preserved because the param_mark is not
23+ # %s.
24+ quoted_statement = re.sub("%", "%%", statement)
25+ quoted_statement = convert_param_marks(
26+ quoted_statement, connection.param_mark, "%s")
27 # We need to massage the query parameters a little to deal with
28 # string parameters which represent encoded binary data.
29 render_params = []
30@@ -132,7 +140,12 @@
31 render_params.append(repr(param.encode('utf8')))
32 else:
33 render_params.append(repr(param))
34- statement_to_log = quoted_statement % tuple(render_params)
35+ try:
36+ statement_to_log = quoted_statement % tuple(render_params)
37+ except TypeError:
38+ statement_to_log = \
39+ "Unformattable query: %r with params %r." % (
40+ statement, query_params)
41 self._expanded_raw_execute(connection, raw_cursor, statement_to_log)
42
43 def _expanded_raw_execute(self, connection, raw_cursor, statement):
44@@ -146,26 +159,29 @@
45 For more information on timelines see the module at
46 http://pypi.python.org/pypi/timeline.
47
48- The timeline to use is obtained via a threading.local lookup -
49- self.threadinfo.timeline. If TimelineTracer is being used in a thread
50- pooling environment, you should set this to the timeline you wish to
51- accumulate actions against before making queries.
52+ The timeline to use is obtained by calling the timeline_factory supplied to
53+ the constructor. This simple function takes no parameters and returns a
54+ timeline to use. If it returns None, the tracer is bypassed.
55 """
56
57- def __init__(self, prefix='SQL-'):
58+ def __init__(self, timeline_factory, prefix='SQL-'):
59 """Create a TimelineTracer.
60
61+ #param timeline_factory: A factory function to produce the timeline to
62+ record a query against.
63 @param prefix: A prefix to give the connection name when starting an
64 action. Connection names are found by trying a getattr for 'name'
65 on the connection object. If no name has been assigned, '<unknown>'
66 is used instead.
67 """
68 super(TimelineTracer, self).__init__()
69+ self.timeline_factory = timeline_factory
70+ self.prefix = prefix
71+ # Stores the action in progress in a given thread.
72 self.threadinfo = threading.local()
73- self.prefix = prefix
74
75 def _expanded_raw_execute(self, connection, raw_cursor, statement):
76- timeline = getattr(self.threadinfo, 'timeline', None)
77+ timeline = self.timeline_factory()
78 if timeline is None:
79 return
80 connection_name = getattr(connection, 'name', '<unknown>')
81
82=== modified file 'tests/tracer.py'
83--- tests/tracer.py 2011-09-12 10:24:04 +0000
84+++ tests/tracer.py 2011-09-13 04:08:34 +0000
85@@ -361,6 +361,19 @@
86 [(conn, 'cursor', "SELECT * FROM person where name = 'VAR1'")],
87 tracer.calls)
88
89+ def test_qmark_percent_s_literal_preserved(self):
90+ """With ? parameters %s in the statement can be kept intact."""
91+ tracer = self.LoggingBaseStatementTracer()
92+ conn = self.StubConnection()
93+ var1 = MockVariable(1)
94+ tracer.connection_raw_execute(
95+ conn, 'cursor',
96+ "SELECT * FROM person where id > ? AND name LIKE '%s'", [var1])
97+ self.assertEqual(
98+ [(conn, 'cursor',
99+ "SELECT * FROM person where id > 1 AND name LIKE '%s'")],
100+ tracer.calls)
101+
102 def test_int_variable_as_int(self):
103 """Int parameters are formatted as an int literal."""
104 tracer = self.LoggingBaseStatementTracer()
105@@ -386,21 +399,37 @@
106 "LIKE '%%' || 'substring' || '-suffix%%'")],
107 tracer.calls)
108
109+ def test_unformattable_statements_are_handled(self):
110+ tracer = self.LoggingBaseStatementTracer()
111+ conn = self.StubConnection()
112+ var1 = MockVariable(u'substring')
113+ tracer.connection_raw_execute(
114+ conn, 'cursor', "%s %s",
115+ [var1])
116+ self.assertEqual(
117+ [(conn, 'cursor',
118+ "Unformattable query: '%s %s' with params [u'substring'].")],
119+ tracer.calls)
120+
121
122 class TimelineTracerTest(TestHelper):
123
124 def is_supported(self):
125 return timeline is not None
126
127+ def factory(self):
128+ self.timeline = timeline.Timeline()
129+ return self.timeline
130+
131 def test_separate_tracers_own_state(self):
132 """"Check that multiple TimelineTracer's could be used at once."""
133- tracer1 = TimelineTracer()
134- tracer2 = TimelineTracer()
135- tracer1.threadinfo.timeline = 'foo'
136- self.assertEqual(None, getattr(tracer2.threadinfo, 'timeline', None))
137+ tracer1 = TimelineTracer(self.factory)
138+ tracer2 = TimelineTracer(self.factory)
139+ tracer1.threadinfo.action = 'foo'
140+ self.assertEqual(None, getattr(tracer2.threadinfo, 'action', None))
141
142 def test_error_finishes_action(self):
143- tracer = TimelineTracer()
144+ tracer = TimelineTracer(self.factory)
145 action = timeline.Timeline().start('foo', 'bar')
146 tracer.threadinfo.action = action
147 tracer.connection_raw_execute_error(
148@@ -408,27 +437,25 @@
149 self.assertNotEqual(None, action.duration)
150
151 def test_success_finishes_action(self):
152- tracer = TimelineTracer()
153+ tracer = TimelineTracer(self.factory)
154 action = timeline.Timeline().start('foo', 'bar')
155 tracer.threadinfo.action = action
156 tracer.connection_raw_execute_success(
157 'conn', 'cursor', 'statement', 'params')
158 self.assertNotEqual(None, action.duration)
159
160- def test_finds_timeline_from_threadinfo(self):
161- """tracer.threadinfo.timeline is writable as part of the API."""
162- tracer = TimelineTracer()
163- tracer.threadinfo.timeline = timeline.Timeline()
164+ def test_finds_timeline_from_factory(self):
165+ factory_result = timeline.Timeline()
166+ factory = lambda:factory_result
167+ tracer = TimelineTracer(lambda:factory_result)
168 tracer._expanded_raw_execute('conn', 'cursor', 'statement')
169- self.assertEqual(1, len(tracer.threadinfo.timeline.actions))
170+ self.assertEqual(1, len(factory_result.actions))
171
172 def test_action_details_are_statement(self):
173 """The detail in the timeline action is the formatted SQL statement."""
174- tracer = TimelineTracer()
175- tracer.threadinfo.timeline = timeline.Timeline()
176+ tracer = TimelineTracer(self.factory)
177 tracer._expanded_raw_execute('conn', 'cursor', 'statement')
178- self.assertEqual(
179- 'statement', tracer.threadinfo.timeline.actions[-1].detail)
180+ self.assertEqual('statement', self.timeline.actions[-1].detail)
181
182 def test_category_from_prefix_and_connection_name(self):
183 class StubConnection(Connection):
184@@ -438,24 +465,18 @@
185 self._event = None
186 self._raw_connection = None
187 self.name = 'Foo'
188- tracer = TimelineTracer(prefix='bar-')
189- tracer.threadinfo.timeline = timeline.Timeline()
190+ tracer = TimelineTracer(self.factory, prefix='bar-')
191 tracer._expanded_raw_execute(StubConnection(), 'cursor', 'statement')
192- self.assertEqual(
193- 'bar-Foo', tracer.threadinfo.timeline.actions[-1].category)
194+ self.assertEqual('bar-Foo', self.timeline.actions[-1].category)
195
196 def test_unnamed_connection(self):
197 """A connection with no name has <unknown> put in as a placeholder."""
198- tracer = TimelineTracer(prefix='bar-')
199- tracer.threadinfo.timeline = timeline.Timeline()
200+ tracer = TimelineTracer(self.factory, prefix='bar-')
201 tracer._expanded_raw_execute('conn', 'cursor', 'statement')
202- self.assertEqual(
203- 'bar-<unknown>', tracer.threadinfo.timeline.actions[-1].category)
204+ self.assertEqual('bar-<unknown>', self.timeline.actions[-1].category)
205
206 def test_default_prefix(self):
207 """By default the prefix "SQL-" is added to the action's category."""
208- tracer = TimelineTracer()
209- tracer.threadinfo.timeline = timeline.Timeline()
210+ tracer = TimelineTracer(self.factory)
211 tracer._expanded_raw_execute('conn', 'cursor', 'statement')
212- self.assertEqual(
213- 'SQL-<unknown>', tracer.threadinfo.timeline.actions[-1].category)
214+ self.assertEqual('SQL-<unknown>', self.timeline.actions[-1].category)

Subscribers

People subscribed via source and target branches

to status/vote changes: