search

Wikia / opentracing-php

MIT License
11 stars 2 forks source link

Add Logger accessor to Span #4

Open wladekb opened 8 years ago

wladekb commented 8 years ago

Reference implementations should be idiomatic in that language. I think the LoggerInterface is so popular we could add support for it in the core of the reference implementation.

I also thought about making the Span object itself implement those methods but we have a conflict between definition of OpenTracing's and LoggerInterface's log method.

I'd be happy to hear your opinion. /cc @beberlei @bensigelman @yurishkuro

bhs commented 8 years ago

@wladekb see https://github.com/opentracing/opentracing.github.io/issues/30 ... I would also like to support a straightforward debug logging interface and feel confident that something to that effect will happen soon (across all platforms). That said, I would vote for doing something simple on each platform and avoid a kitchen-sink logging interface. I'm not familiar with LoggerInterface (since I'm not familiar with PHP) so can't speak to its kitchen-sink-ness.

beberlei commented 8 years ago

@wladekb I think this is not needed, because its implementation specific and this PR adds no benefit, just more public code. Essentially $span->getLogger()->... would just be $span->log(...); so why not just keep $span->log().

Every tracer has to decide how to implement logging internally, there could be a PSR based tracer that uses a logging framework such as Monolog, but thats entirely tracer specific.

wladekb commented 8 years ago

@beberlei the log() method from opentracing is missing a $severity argument which is part of the definition in LoggerInterface so they are not compatible. I did a direct translation from Go/Python reference implementation for the log() method and severity is not part of the opentracing standard, thus the ->getLogger() version extended that log() method and put severity into $payload

any thoughts?