reuse python code from datadog_checks_base #1704
Conversation
|
@ofek: could you explain a little more what this PR does and why ? |
There was a problem hiding this comment.
Made a few inline comments, overall LGTM.
A few general comments/questions:
- can you squash your commits?
- can you add a
renorelease note with adeprecationssection mentioning that these python import paths should be changed (in user-provided custom checks)? - Is there a plan to do a similar change in Agent5? If not I think we should also list this deprecation in
docs/agent/changes.md
| def report_as_service_check(self, sc_name, status, instance, msg=None): | ||
| """This function should be implemented by inherited classes""" | ||
| raise NotImplementedError | ||
| from datadog_checks.checks import NetworkCheck, Status |
There was a problem hiding this comment.
let's expose EventType too; any reason not to?
There was a problem hiding this comment.
It's not used anywhere. Shall I add it anyway?
|
|
||
| from prometheus_check import * | ||
| from . functions import parse_metric_family # noqa: F401 | ||
| from datadog_checks.checks.prometheus import PrometheusCheck |
There was a problem hiding this comment.
I think we should also import PrometheusFormat and UnknownFormatError
There was a problem hiding this comment.
They are not used anywhere. Shall I add them anyway?
| @@ -174,16 +174,31 @@ def misspell(ctx, targets): | |||
| print("misspell found no issues") | |||
|
|
|||
| @task | |||
| def deps(ctx): | |||
| def deps(ctx, no_checks=False, core_dir=None, verbose=False): | |||
There was a problem hiding this comment.
Can you document how a developer is supposed to use these options when setting up their dev environment? Could be documented in the command help, might be worth updating the docs/dev/ if applicable.
|
|
||
| if not no_checks: | ||
| verbosity = 'v' if verbose else 'q' | ||
| core_dir = core_dir or os.getenv('DD_CORE_DIR') |
There was a problem hiding this comment.
DD_CORE_DIR should be documented
|
@ofek about the legacy imports that I mentioned in my review: the rationale is to continue supporting the old import paths for custom checks that may still be using them, so I think we should do that for every class/function that could reasonably be imported from a custom check. Ideally, once all the integrations-core checks use the new import paths, we'd log a deprecation warning when an old import path is used so that it's clear to users that they need to update their custom checks' code. |
| - | | ||
| The core Agent check Python code is no longer duplicated here and is instead | ||
| pulled from integrations-core. The code now resides in the `datadog_checks` | ||
| namespace, though the old `checks`, `utils`, etc. paths are still supported. |
There was a problem hiding this comment.
can you add something like: Please update your custom checks accordingly , and add a link to the relevant documentation?
docs/dev/agent_dev_env.md
Outdated
| @@ -27,6 +27,40 @@ You must install [go](https://golang.org/doc/install) version 1.9.2 or above. Ma | |||
| sure that `$GOPATH/bin` is in your `$PATH` otherwise Invoke cannot use any | |||
| additional tool it might need. | |||
|
|
|||
| ### Python | |||
There was a problem hiding this comment.
this Python section mostly talks about installing the dev versions of python needed to build the agent against. So I think it should be left below in the System or Embedded section.
Here, you could simply mention in the Invoke section that a standard python+pip install is required for development maybe? What do you think?
This removes all code that is also in https://github.com/DataDog/integrations-core/tree/master/datadog_checks_base by simply importing what is necessary.
Motivation
Deduplication of logic: We often have to update code in both places at the same time.
depends on:
DataDog/integrations-core#1561 ✔️
DataDog/integrations-core#1562 ✔️
DataDog/integrations-core#1565 ✔️
DataDog/integrations-core#1566 ✔️