search

opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
505 stars 56 forks source link

Don't throw exception on Tracer::inject/extract #81

Closed sergeyklay closed 6 years ago

sergeyklay commented 6 years ago

Short description of what this PR does:

All exceptions thrown from Tracer::inject and Tracer::extract should be caught and logged on WARN level so that business code execution isn't affected. If possible, catch implementation specific exceptions and log more meaningful information.

Checklist

jcchavezs commented 6 years ago

the UnsupportedFormat exception is a logical exception which means that it should be part of static checks and not related to user input. I don't think we should remove it. For RuntimeExceptions I agree we should instead log them.

Den lør. 18. aug. 2018, 10:24 skrev Serghei Iakovlev < notifications@github.com>:

Short description of what this PR does:

  • Don't throw exception on Tracer::inject and Tracer::extract

All exceptions thrown from Tracer::inject and Tracer::extract should be caught and logged on WARN level so that business code execution isn't affected. If possible, catch implementation specific exceptions and log more meaningful information. Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

You can view, comment on, or merge this pull request online at:

https://github.com/opentracing/opentracing-php/pull/81 Commit Summary

  • Don't throw exception on Tracer::inject/extract

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/pull/81, or mute the thread https://github.com/notifications/unsubscribe-auth/AC7sAiPKbSynvcNhoIa1Et-ZjIW8gEwdks5uR88fgaJpZM4WCfCl .

--

José Carlos

sergeyklay commented 6 years ago

I'm pretty sure that tracing shouldn't stop application the same as the autoloader (PSR-4) or, for example the logger. Let's take a look for example at Java Client:

https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/spi/Extractor.java#L33-L50

and

https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/spi/Injector.java#L34-L48

Could you explain why an introduction of opentracing into the project should increase risks in the form of UnsupportedFormat exception?

jcchavezs commented 6 years ago

This document you point out is for the Extractor interface which is being used when you actually extract the context and this is a Runtime thing, of course UnsupportedFormat is not a thing here because the decision on what extractor should be use is made one layer up. What you are looking for here is where the tracer calls the injector/extractor which occurs in https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java#L194 and as you can see it also throws the UnsupportedFormat exception. As I said before, this is something that should be checked statically and does not involve the user input.

Den lør. 18. aug. 2018, 14:18 skrev Serghei Iakovlev < notifications@github.com>:

I'm pretty sure that tracing shouldn't stop application the same as the autoloader (PSR-4) or, for example the logger. Let's take a look for example at Java Client:

https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/spi/Extractor.java#L33-L50

and

https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/spi/Injector.java#L34-L48

Could you explain why an introduction of opentracing into the project should increase risks in the form of UnsupportedFormat exception?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/pull/81#issuecomment-414054138, or mute the thread https://github.com/notifications/unsubscribe-auth/AC7sArWfB8D6K-fL5-1e5hOhLfmRsJmiks5uSAYCgaJpZM4WCfCl .

--

José Carlos

jcchavezs commented 6 years ago

Maybe an example is more clarifier: if the context comes with a wrong format we are not throwing an exception. This exception is being thrown when you attempt (programmatically) to inject/extract a context on binary format but you did not configure such injector/extractor. This exception should show up in development or tests stage, never in production. Think of it as passing a wrong type in the signature of a PHP function.

Den lør. 18. aug. 2018, 14:33 skrev José Carlos Chávez <jcchavezs@gmail.com

:

This document you point out is for the Extractor interface which is being used when you actually extract the context and this is a Runtime thing, of course UnsupportedFormat is not a thing here because the decision on what extractor should be use is made one layer up. What you are looking for here is where the tracer calls the injector/extractor which occurs in https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java#L194 and as you can see it also throws the UnsupportedFormat exception. As I said before, this is something that should be checked statically and does not involve the user input.

Den lør. 18. aug. 2018, 14:18 skrev Serghei Iakovlev < notifications@github.com>:

I'm pretty sure that tracing shouldn't stop application the same as the autoloader (PSR-4) or, for example the logger. Let's take a look for example at Java Client:

https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/spi/Extractor.java#L33-L50

and

https://github.com/jaegertracing/jaeger-client-java/blob/b6a824d0f6e54f246a1c5583e45536a1143c4c30/jaeger-core/src/main/java/io/jaegertracing/spi/Injector.java#L34-L48

Could you explain why an introduction of opentracing into the project should increase risks in the form of UnsupportedFormat exception?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/pull/81#issuecomment-414054138, or mute the thread https://github.com/notifications/unsubscribe-auth/AC7sArWfB8D6K-fL5-1e5hOhLfmRsJmiks5uSAYCgaJpZM4WCfCl .

--

José Carlos

--

José Carlos

sergeyklay commented 6 years ago

Thank you