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

Fix Metric.send() to play nice with multiple metrics #56

Closed
wants to merge 2 commits into from

Conversation

kuzmich
Copy link
Contributor

@kuzmich kuzmich commented Jun 2, 2015

In short, method signature and usage (http://docs.datadoghq.com/api/#metrics) is different and misguiding. Also, it will send the wrong data if user pass list of metrics with points as a single value. Below is a detailed explanation.

Here's a Metric.send() method without docstrings:

    @classmethod
    def send(cls, *metrics, **single_metric):
        try:
            if metrics:
                for metric in metrics:
                    if isinstance(metric, dict):
                        metric['points'] = cls._process_points(metric['points'])
                metrics_dict = {"series": metrics[0]}
            else:
                single_metric['points'] = cls._process_points(single_metric['points'])
                metrics = [single_metric]
                metrics_dict = {"series": metrics}

        except KeyError:
            raise KeyError("'points' parameter is required")

        return super(Metric, cls).send(attach_host_name=True, **metrics_dict)

There're few issues with it:

  1. Method signature says it accepts arbitrary number of metrics, but api docs says it should be used with a single list, not multiple positional args. It really misguiding users.
  2. If you pass a list of metrics as a single positional argument, metrics will be a list of a sinle list ([[your metrics here]]). Let's see what happens next.
    for metric in metrics will take a list of metrics and compare it with a dict - indeed, list is not a dict, so no calls to cls._process_points(). It will absolutely not change metrics in any way. Since points must be a list of lists of timestamp and a value [[timestamp, value]], wrong data will be sent. Though datadog api will say 202 Accepted, it will not not accept any data at all.
  3. We can pass metrics as arbitrary args (Metric.send(*metrics)), and it will call _process_points() and transform data in the right way. But then it will take only first metric (it will be a dict), and since series must be a list, not a dict, we will get an error. And that's what I changed - 1 line of code.

@robinske
Copy link

+1, running into this problem for integrating with Runscope API tests. Any estimate on getting this added?

@remh
Copy link
Contributor

remh commented Jun 19, 2015

@yannmh can you have a look into this one please ?

@yannmh
Copy link
Member

yannmh commented Jun 22, 2015

Thanks a lot @kuzmich for the very detailed explanations !

1 - You are right, it's a bit confusing. While the documentation attests that the method can be used only with a single list, the examples show the same method being used with a single list and positional arguments.
I'll update the documentation to reflect the actual method state .

2 & 3 - I rebased your work, and added an extra commit on top of it to comply with the syntax demoed in the documentation examples (http://docs.datadoghq.com/api/#metrics), i.e. pass a list of metrics as a single positional argument.

Metric.send(metrics)

instead of

Metric.send(*metrics)

I opened a new PR here: #59. Thanks again

@robinske we can release it this week.

@robinske
Copy link

thanks @yannmh, ended up writing the integration without the sdk

@yannmh
Copy link
Member

yannmh commented Jun 25, 2015

Merged with #59. Thanks again @kuzmich.

@yannmh yannmh closed this Jun 25, 2015
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

Successfully merging this pull request may close these issues.

4 participants