Hostname lookup in debug and on/off setup

[dabcoder]

Attempts to address #416.

[bkabrda]

I'm wondering whether it might be better to add an argument to get_hostname, e.g. def get_hostname(from_config=True), so that people using this from outside of the library could use it better (while preserving current behavior). Additionally, I see you added the argument to the docstring of initialize, but I don't actually see the argument there - perhaps you forgot about it?

[dabcoder]

I'm wondering whether it might be better to add an argument to get_hostname, e.g. def get_hostname(from_config=True), so that people using this from outside of the library could use it better (while preserving current behavior).

That is a possibility, good point. Will look into that.

Additionally, I see you added the argument to the docstring of initialize, but I don't actually see the argument there - perhaps you forgot about it?

No I was looking at other optional arguments such as cacert:

datadogpy/datadog/__init__.py

Lines 63 to 66 in 42b578c

	:param cacert: Path to local certificate file used to verify SSL \
	certificates. Can also be set to True (default) to use the systems \
	certificate store, or False to skip SSL verification
	:type cacert: path or boolean

but it's not used anywhere in the initialize method either, so was not sure how to reference it.

[bkabrda]

I'm not sure about the cacert specifically, but we should either:

• Make this a global configuration variable and then not pass it to initialize.
• Make this an argument to initialize, thus passing it to get_hostname, which is called inside initialize.

[apenney]

Just a bump to ask how this one is going?

I was reminded this morning when adding a new log source and realizing I see over 250,000 of these errors a day in our logs.

[jd]

Clearly log.info was not a good choice here. :)

[apenney]

You're the best!

[bkabrda]

Thanks for the additional work, @dabcoder. Some points (trying to make my last comment clearer):

• As mentioned in my previous comment, I'd prefer if this was configurable through initialize (so add a new argument hostname_from_config=True, so that users can set that to False and try what their app will do in future when we remove this completely.
• I think the warning should be emitted on log.warning level, additionally recommending that the user should call initialize with hostname_from_config=False to get rid of this warning. This will make sure that users who want to get rid of this warning explicitly consent to not using config while users who want to keep trying to use the config will get warned when the config is not accessible.

Does that sound good to everyone? (@apenney would appreciate your feedback on this)

[bkabrda]

I'd honestly put the new argument at the very end of the argument list, because it someone is doing right now initialize("xxx", "yyy", None, "some_api_host"), this would break their code. Optional arguments should always be added to the end.

[dabcoder]

👍 Done in the last commit

[bkabrda]

/azp run DataDog.datadogpy.integration

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[bkabrda]

LGTM now, merging. Thanks!