Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling exceptions with the Noop Tracer #216

Closed
jshayes opened this issue Jan 6, 2019 · 7 comments
Closed

Error handling exceptions with the Noop Tracer #216

jshayes opened this issue Jan 6, 2019 · 7 comments

Comments

@jshayes
Copy link

jshayes commented Jan 6, 2019

I have received the following error in some of my tests.

Symfony\Component\Debug\Exception\FatalThrowableError: Call to undefined method OpenTracing\NoopSpan::setError()

When my tests are run, php is running as a cli, as a result the tracer is not registered in the service provider. I am using Laravel, and one of my tests hits a route and in that particular flow an exception is thrown in my application and bubbles up to the exception handler. It seems like this exception causes an issue with some of the registered trace functions as the setError method is missing.

It looks like in DDTrace\Integrations\Laravel\V5\LaravelProvider, if php is running in cli it will not run the register method, but it will still run the boot method. So, no tracer is created, and it seems to default to the OpenTracing\Tracer. I noticed that there is a way to create a noop tracer in the DDTrace\Tracer class, but if I register that tracer in the case where php is run as a cli, I get the following error

Error: Call to undefined method DDTrace\NoopSpan::getService()

So, it seems like both of the noop tracers are missing some methods.

@SammyK
Copy link
Contributor

SammyK commented Jan 7, 2019

Thanks so much for the detailed report @jshayes! I just submitted #220 which should hopefully fix this issue. I'm hoping we can squeeze this in before today's release. :)

@jshayes
Copy link
Author

jshayes commented Jan 7, 2019

Awesome, that was quick 😄. Thanks!

@labbati
Copy link
Member

labbati commented Jan 9, 2019

@jshayes did the fix work for you?

@jshayes
Copy link
Author

jshayes commented Jan 9, 2019

@labbati Yes it did 😄, thanks.

The only thing that concerns me about it is that before the boot method would still run during my tests, which would register all the dd_trace functions and execute the code within the callbacks when those particular methods were executed. But now since the boot method it not run at all, none of that code is executed in my test suite. So any errors introduced into that callbacks won't cause my test suite to fail, even though those errors would be present when using the application. Any issues in these callbacks can be found with some manual testing, but I did notice that it would be more difficult to ensure that everything is still working correctly with automated tests.

@labbati
Copy link
Member

labbati commented Jan 11, 2019

@jshayes thanks for confirming that your issue is now gone.

On a different topic I see what you mean and currently we do not offer a way for you to test your code . with our integrations enabled, but we should. You made a very good point.

Supporting this on our side would be not too complicated.

The reason why integrations don't get loaded is because is we detect the execution is of type cli, we only enable integrations if we detect an environment named dd_testing. We could change this behavior to a generic setting, e.g. through an env DD_ENABLE_CLI=true.

This, in addition to use the DebugTracer that we already provide should be enough for users to test their code with our integration loaded.

What do you thing? Would it work for you?

@jshayes
Copy link
Author

jshayes commented Jan 12, 2019

I was initially looking at setting the env to dd_testing so that I could get the code running in my tests but having an option to enable it through the environment, like DD_ENABLE_CLI=true would be awesome. It sounds like this would work well for me.

@pawelchcki
Copy link
Contributor

pawelchcki commented Feb 1, 2019

This should have been fixed in 0.10.0 because we've migrated away from requiring OpenTracing API.
Feel free to reopen if the problem still persists. Thanks!

As for allowing running the integrations in more SAPIs - its currently on our radar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants