fix: don't allow package version to be None

[Kyle-Verhoog]

If users try to repackage the library there is the possibility that the __version__ can be None which eventually causes the writer to fail as it tries to serialize None into the http headers sent to the agent.

This PR provides a more sensible default.

thanks to @thehesiod for finding and describing this error 🙂.

[thehesiod]

thank you!

[brettlangdon]

hmm, when does this happen?

[Kyle-Verhoog]

@brettlangdon it was occurring in @thehesiod's case in a pretty niche situation where the version would be None as a result of

dd-trace-py/ddtrace/__init__.py

Lines 14 to 18 in 14fb771

	try:
	__version__ = pkg_resources.get_distribution(__name__).version
	except pkg_resources.DistributionNotFound:
	# package is not installed
	__version__ = None

which would then be used here*:

dd-trace-py/ddtrace/api.py

Line 156 in 14fb771

'Datadog-Meta-Tracer-Version': ddtrace.__version__,

* edited to correct the line

[brettlangdon]

Makes sense, that is off scenario, but possible.

aside: I wonder if we should default to "dev" instead of None /cc @jd

[jd]

Just one question: the reason you were able to fix that headers issue is because the Exception base class is not caught.

You're now hiding that bug under the carpet IIUC.

I don't see how it's wise?

If you want the writer thread to be robust to crash, you should handle a general try/except Exception at its root that makes it restart on handled exception. Using except Exception everywhere and hiding issues is not goint to help us in the long run.

[Kyle-Verhoog]

@jd agreed. But you're proposing a larger refactoring change (which I totally agree with). This is correction to existing code that, from what I can tell, assumed that it was catching all possible exceptions that could be raised and it wasn't.

Given the way the current code is structured I think it makes sense. I also don't like it either. A larger change to this code will take longer to review/get merged whereas this addresses the problem immediately without introducing any new "badness" that was not already there 😛.

Exception code aside, I think it does make sense to change the default for version to be, at least, not None.

Your thoughts? 🙂

[jd]

The problem is that I think the medicine is worse than the illness: this is not going to fix the illness, it's going to hide it, and worse, hide future ones!

I'm not asking for a large refactoring. I'm stating the current behavior is sane. What should be fixed is:

1. the header that is None should be fixed (Brett proposed a solution for this)
2. if we want to make the code more robust we shouldn't do what this PR does but make the worker thread restart-on-crash (I hope it's not a large refactoring!)

[Kyle-Verhoog]

Fair enough 🙂. The real issue at hand here is that a header can be None so let's correct that. I've got some changes in the pipeline for how the worker deals with exceptions so we can revisit the design there.

[thehesiod]

in general assuming what errors can occur from a global error handler is bad policy, especially w/o any sort of logging that errors are uncaught since it causes the thread to go away