Skip to content

Conversation

@bmermet
Copy link
Contributor

@bmermet bmermet commented Aug 24, 2017

This fix #304

I have tested locally that it fixes the bug, but I haven't added a unit test since I haven't found a simple way to do it. It requires running the unit tests with a different config at startup and I'm not convinced it's worth adding new test envs only to check on this.

@bmermet bmermet requested a review from palazzem August 24, 2017 17:31
@palazzem palazzem added the bug label Aug 28, 2017
@palazzem palazzem added this to the 0.9.2 milestone Aug 28, 2017
Copy link

@palazzem palazzem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! About the test, as we've discussed "offline", you can write a test reloading the application in that way:

from django.apps import apps

def test_service_not_called(self):
    # mock/spy the tracer so that you know if the service endpoint has been called or not

    # change our local settings because they've been initialized and the Django decorator will not work in this case
    settings.ENABLED = False
    # reload the application
    app = apps.get_app_config('datadog_django')
    app.ready()
    # now you can check if the service method has been called or not

Also it's worth looking if with this approach ^^ you should restore and re-initialize the app. If this is the case, could be interesting to write a decorator for that so it can be reused. I'd say to do that in another PR later on if we have to.

app = apps.get_app_config('datadog_django')
app.ready()

traces = tracer.writer.pop_traces()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the test is nothing_is_written, you can add a tracer.trace() call to create and finish a Span. Calling app.ready() will only send services and not traces, so adding something could be great. If you prefer, you can split the test in two: one for services and one for traces.

Copy link

@palazzem palazzem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!! Good to me! Good job!

@palazzem palazzem merged commit bb2f5a2 into master Aug 30, 2017
@palazzem palazzem deleted the bemermet/bug-sendtraceswhendisabled branch August 30, 2017 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error log still occurs when tracer is disabled (Django)

3 participants