[core] Submitting metrics to the api endpoint #3180

remh · 2017-02-08T23:34:17Z

What does this PR do?

This PR splits the collector payload into a legacy payload and a metrics payload.

It does so by extracting the metrics that are in the "new" format from the current payload, transforming them, a putting them into a new payload to be posted to the api/v1/series endpoint.

Motivation

This will help up phasing out a backend service used by the old payload.

Testing Guidelines

Added a unit test on the split function
Tested that this was a no op on my agent (i.e. all metrics keep flowing seamlessly)
This will need to be tested at higher scale as well during QA.

Add a test Add support for device

yannmh · 2017-02-27T16:57:00Z

tests/core/test_emitter.py

+            formatted_sample = [s['metric'], s['points'][0][0], s['points'][0][1], attributes]
+            legacy_payload_split['metrics'].append(formatted_sample)
+
+        self.assertEqual(legacy_payload, legacy_payload_split)


This will always be True, split_payload modifies the original payload by reference, in other words

# &legacy_payload_split == &legacy_payload legacy_payload_split, metrics_payload = split_payload(legacy_payload)

yes, and so that things are clearer maybe split_payload should either return a legacy_payload_split and not mutate the legacy_payload passed as argument or not return a legacy_payload_split and mutate the legacy_payload (which probably makes more sense here)

good catch 🤦‍♂️

i'll just pass a copy of the payload to the test for 2 reasons.

Better to be explicit, if the function is mutating the dict, having it being returned is better than silently mutating it.

We could make a copy of the dict in the split payload function but this would come with a memory cost.

So i'll just make a copy of the payload in the test function.
Good catch

yannmh · 2017-02-27T17:50:28Z

emitter.py

+
+        metrics_payload["series"].append(sample)
+
+    return legacy_payload, metrics_payload


Naming nitpick: legacy_payloads refers to both the original input, and the modified/stripped output. Shall we use two different names?

it doesn't matter and it's still a legacy payload

yannmh · 2017-02-27T18:00:36Z

emitter.py

+                sample['tags'] = ts[3]['tags']
+            if ts[3].get('device_name'):
+                sample['device'] = ts[3]['device_name']
+


We are not handling any exception in this block.
Considering that metrics are strongly formatted, I believe it is "safe" to accept it and avoid a try ... except ... continue overhead.

@olivielpeau any thoughts?

not handling exceptions here looks safe to me, the format is well-defined and the code is safe

Yes this code is safe.

yannmh · 2017-02-27T18:02:50Z

emitter.py



-def post_headers(agentConfig, payload):
+def split_payload(legacy_payload):
+    metrics = list(legacy_payload['metrics'])


Nitpick: the method performs reads only on metrics, thus, it is not required to make a copy of it.

yannmh

The test needs a re-write but the overall logic looks good to me.

I'd also perform a simple benchmark to measure what overhead this adds in the collector.

yannmh · 2017-02-27T18:26:12Z

emitter.py

+            if ts[3].get('tags'):
+                sample['tags'] = ts[3]['tags']
+            if ts[3].get('device_name'):
+                sample['device'] = ts[3]['device_name']


We could change those keys directly in the MetricAggregator formatter to speed things up.

sample.update(ts[3])

should device_name be changed to device? Dogstatsd uses device_name in its API payload (https://github.com/DataDog/dd-agent/blob/5.11.3/aggregator.py#L991)

@yann i didn't want to change the formatter as i thought it was used by the legacy checks. Turns out it's not the case. I'll update this.

@olivielpeau what do you mean ? legacy payload processing will use device_name, new payload expects device hence my change.

Actually i want to keep this logic kinda isolated as it's a hack. Better to keep it in a single place to keep the code cleaner. Agent 6 will get rid of all of this anyway.

olivielpeau

LGTM overall, added a few comments.

Just as a side note, it looks like handling the api series format directly in the new-style and old-style check base classes would be a bit tricky since the unix system metrics can't use the api series endpoint

olivielpeau · 2017-02-27T21:25:36Z

emitter.py

+    for ts in metrics:
+        sample = {
+            "metric": ts[0],
+            "points": [[ts[1], ts[2]]]


nit: could be a tuple instead of a list

olivielpeau · 2017-02-27T21:36:21Z

emitter.py

+            if ts[3].get('tags'):
+                sample['tags'] = ts[3]['tags']
+            if ts[3].get('device_name'):
+                sample['device'] = ts[3]['device_name']


should device_name be changed to device? Dogstatsd uses device_name in its API payload (https://github.com/DataDog/dd-agent/blob/5.11.3/aggregator.py#L991)

olivielpeau · 2017-02-27T21:40:52Z

emitter.py

+                sample['tags'] = ts[3]['tags']
+            if ts[3].get('device_name'):
+                sample['device'] = ts[3]['device_name']
+


not handling exceptions here looks safe to me, the format is well-defined and the code is safe

olivielpeau · 2017-02-27T21:48:44Z

tests/core/test_emitter.py

+            formatted_sample = [s['metric'], s['points'][0][0], s['points'][0][1], attributes]
+            legacy_payload_split['metrics'].append(formatted_sample)
+
+        self.assertEqual(legacy_payload, legacy_payload_split)


yes, and so that things are clearer maybe split_payload should either return a legacy_payload_split and not mutate the legacy_payload passed as argument or not return a legacy_payload_split and mutate the legacy_payload (which probably makes more sense here)

- Do not make a copy of the metrics - Pass a copy to the function in the test

remh · 2017-02-27T23:27:21Z

Addressed some of your comments and fixed a few things. Let me know if there is other stuff but i think we're good now.

[core] First pass at submitting series to the api endpoint

6af3068

remh added this to the 5.12.0 milestone Feb 8, 2017

Remi Hakim added 2 commits February 24, 2017 17:42

[core] Payloads split

21027e8

Add a test Add support for device

formatting

0be9f9e

remh requested review from olivielpeau and yannmh February 24, 2017 22:48

pep8

be22ca6

remh changed the title ~~[core] First pass at submitting series to the api endpoint~~ [core] Submitting metrics to the api endpoint Feb 24, 2017

remh requested a review from jmoiron February 24, 2017 22:57

Remi Hakim added 3 commits February 24, 2017 17:58

pep8

05fe939

more pep8

ad68f98

Better method name

b783555

yannmh reviewed Feb 27, 2017

View reviewed changes

olivielpeau reviewed Feb 27, 2017

View reviewed changes

Fix after review

b025af9

- Do not make a copy of the metrics - Pass a copy to the function in the test

yannmh approved these changes Feb 28, 2017

View reviewed changes

remh merged commit b411be1 into master Feb 28, 2017

remh deleted the remh/split_payloads branch February 28, 2017 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Submitting metrics to the api endpoint #3180

[core] Submitting metrics to the api endpoint #3180

remh commented Feb 8, 2017 •

edited

Loading

yannmh Feb 27, 2017

olivielpeau Feb 27, 2017

remh Feb 27, 2017

remh Feb 27, 2017

yannmh Feb 27, 2017

remh Feb 27, 2017

yannmh Feb 27, 2017

olivielpeau Feb 27, 2017

remh Feb 27, 2017

yannmh Feb 27, 2017

yannmh left a comment

yannmh Feb 27, 2017 •

edited

Loading

olivielpeau Feb 27, 2017

remh Feb 27, 2017

remh Feb 27, 2017

olivielpeau left a comment

olivielpeau Feb 27, 2017

remh Feb 27, 2017

olivielpeau Feb 27, 2017

olivielpeau Feb 27, 2017

olivielpeau Feb 27, 2017

remh commented Feb 27, 2017


		metrics_payload["series"].append(sample)

		return legacy_payload, metrics_payload

[core] Submitting metrics to the api endpoint #3180

[core] Submitting metrics to the api endpoint #3180

Conversation

remh commented Feb 8, 2017 • edited Loading

What does this PR do?

Motivation

Testing Guidelines

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannmh left a comment

Choose a reason for hiding this comment

yannmh Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivielpeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remh commented Feb 27, 2017

remh commented Feb 8, 2017 •

edited

Loading

yannmh Feb 27, 2017 •

edited

Loading