api: change order of arguments in add_event#270
Conversation
e4d8949 ("OpenTracing Bridge - Initial implementation (open-telemetry#211)") introduced a new timestamp argument to the add_event method. This commit moves that argument to be the last one because it is more common to have event attributes than an explicitly timestamp.
mauriciovasquezbernal
requested review from
Oberon00,
a-feld,
c24t,
carlosalberto,
lzchen,
reyang and
toumorokoshi
as code owners
c24t
left a comment
c24t
left a comment
There was a problem hiding this comment.
The changes LGTM, but adding a few lines to test_span_members pushed it over the threshold for pylint to raise a too-many-statements.
We may want to disable this check eventually, but I think pylint is right in this case: this test would benefit from being broken up.
toumorokoshi
left a comment
toumorokoshi
left a comment
There was a problem hiding this comment.
+1 on doing something to break up the tests vs removing the linting. But LGTM! Assuming that gets fixed approved.
|
|
||
| event_name = util.event_name_from_kv(key_values) | ||
| self._otel_span.add_event(event_name, event_timestamp, key_values) | ||
| self._otel_span.add_event(event_name, key_values, event_timestamp) |
There was a problem hiding this comment.
I'd personally use explicit param names here, in order to be on the safe side (in case we change order in the future ;) )
There was a problem hiding this comment.
Actually having the parameters by position could be good because it can be a reminder that if the position is changed something could break.
c24t
left a comment
c24t
left a comment
There was a problem hiding this comment.
Thanks @mauriciovasquezbernal, LGTM!
The flask integration has (only) two advantages over the plain WSGI middleware approach: - It can use the endpoint as span name (which is lower cardinality than the route; cf #270) - It can set the http.route attribute. In addition, it also has an easier syntax to enable (you don't have to know about Flask.wsgi_app).
4 participants
e4d8949 ("OpenTracing Bridge - Initial implementation (#211)") introduced
a new timestamp argument to the add_event method. This commit moves that
argument to be the last one because it is more common to have event attributes
than an explicitly timestamp.