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

Dogshell.dog service_check, monitor includes null values #206

Closed
ronindesign opened this issue May 20, 2017 · 1 comment
Closed

Dogshell.dog service_check, monitor includes null values #206

ronindesign opened this issue May 20, 2017 · 1 comment
Milestone

Comments

@ronindesign
Copy link
Contributor

ronindesign commented May 20, 2017

dog script's service_check, monitor options are not required, but are still sent (as null) when not provided. Assuming this is the behavior for all script commands.

Reproduce:

$ dog service_check check check_pg host0 0
ERROR: "message" parameter should be a string

The actual payload / body sent to server includes nulled options, when not specified.

{"status": 0, "tags": null, "timestamp": null, "host_name": "host0", "message": null, "check": "check_pg"}

Relates:
https://github.com/DataDog/datadogpy/blob/master/datadog/dogshell/service_check.py#L32
https://github.com/DataDog/datadogpy/blob/master/datadog/api/service_checks.py#L39
https://github.com/DataDog/datadogpy/blob/master/datadog/api/api_client.py#L112
https://github.com/DataDog/datadogpy/blob/master/datadog/api/api_client.py#L127

This could be fixed locally in datadog.dogshell.service_checkto not submitNoneoptions; or this could be fixed indatadog.api.service_checksordatadog.api.api_client`.

The first would have the most localized effect / least possibility for wider side effects to other tests, but if there are no cases for submitting null values to the DataDog API, then I would definitely recommend mitigating this directly in datadog.api.api_client.

@ronindesign ronindesign changed the title Dogshell.dog service_check includes null values Dogshell.dog service_check, monitor includes null values May 20, 2017
@yannmh yannmh added this to the 0.17.0 milestone May 24, 2017
@timvisher
Copy link
Contributor

The commits that fixed this introduced a python error:

root@tvisher1:~# dog service_check check check_filebeat "$(hostname)" 2
Traceback (most recent call last):
  File "/usr/local/bin/dog", line 11, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.4/dist-packages/datadog/dogshell/__init__.py", line 69, in main
    args.func(args)
  File "/usr/local/lib/python3.4/dist-packages/datadog/dogshell/service_check.py", line 34, in _check
    timestamp=args.timestamp, message=args.message, tags=args.tags)
  File "/usr/local/lib/python3.4/dist-packages/datadog/api/service_checks.py", line 37, in check
    for param, value in params.items():
RuntimeError: dictionary changed size during iteration
r

I applied the following patch and it worked well.

--- /usr/local/lib/python3.4/dist-packages/datadog/api/service_checks.orig.py   2017-11-10 11:00:23.925812101 -0500
+++ /usr/local/lib/python3.4/dist-packages/datadog/api/service_checks.py        2017-11-10 11:00:43.198480330 -0500
@@ -35,9 +35,7 @@

         # Validate checks, include only non-null values
         for param, value in params.items():
-            if value is None:
-                del params[param]
-            elif param == 'status' and params[param] not in CheckStatus.ALL:
+            if param == 'status' and params[param] not in CheckStatus.ALL:
                 raise ApiError('Invalid status, expected one of: %s'
                                % ', '.join(str(v) for v in CheckStatus.ALL))

I'll try to submit a PR.

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

3 participants