ext/jaeger: fix example #262
Conversation
The example of the jaeger exporter is failing because e4d8949 ("OpenTracing Bridge - Initial implementation (open-telemetry#211)") introduced a new timestamp parameter to add_event, the example was passing parameter by position so it was messed up.
mauriciovasquezbernal
requested review from
a-feld,
c24t,
carlosalberto,
lzchen,
Oberon00,
reyang and
toumorokoshi
as code owners
|
Good catch! Actually, I think that change in #211 was a mistake: the timestamp should be added to the end of the argument list, as using a timestamp should be a nonstandard use case and not encouraged. On the other hand, I imagine event attributes are quite common. I'd prefer to fix this in That could be catched in CI if we had mypy checks for that example 😉 (although #201 seems to show that mypy checks are not really feasible everywhere) |
| @@ -31,8 +31,8 @@ | |||
| # create some spans for testing | |||
| with tracer.start_as_current_span("foo") as foo: | |||
| time.sleep(0.1) | |||
| foo.set_attribute("my_atribbute", True) | |||
| foo.add_event("event in foo", {"name": "foo1"}) | |||
| foo.set_attribute("my_attribute", True) | |||
There was a problem hiding this comment.
Either solution LGTM, but I agree with @Oberon00 that it makes sense to change the arg order. FWIW that's also the argument order that @carlosalberto picked in java.
|
I'll update this PR/open a new one changing the order of the arguments. |
|
I just opened a PR with the new approach #270, when that is merged we can close this one. |
|
Closing this one in favor of #270 |
closes open-telemetry#165 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
The example of the jaeger exporter is failing because
e4d8949 ("OpenTracing Bridge - Initial implementation (#211)") introduced
a new timestamp parameter to add_event, the example was passing parameter by
position so it was messed up.
I tried to add this to the CI but didn't find an easy way to do it, if somebody has a suggestion it's welcome!