Skip to content
This repository has been archived by the owner on Mar 15, 2021. It is now read-only.

Add distribution support to ffwd agent modules #221

Merged
merged 11 commits into from
Nov 17, 2020
Merged

Add distribution support to ffwd agent modules #221

merged 11 commits into from
Nov 17, 2020

Conversation

ao2017
Copy link
Contributor

@ao2017 ao2017 commented Oct 13, 2020

Heroic histogram data is currently computed locally.
It is practically impossible to aggregate percentile.
We are adding distribution to heroic to address that issue.
Distribution will create data sketch that will be used upstream to compute percentile on the entire data distribution.

This PR affects many files but some of the change simply change the metric type in the import statement.
The changes that are consequential are described below and in the attached document.

Metric Data Model
We changed the data point value type to Value and eliminate unused fields.
This change will have no performance implication unless users start sending distribution metric.

Serialization:
Even though ffwd will continue to process old version of metric, it will only forward metric into new version only.
There is a PR pending that will handle the new metric format in Heroic consumer.
We don't expect any performance implication related to this change since input metric conversion is not new.

ProtoBuf Module
Has been updated and tested to support distribution

Http Module
Has been updated to handle both new and old metric.
Dev for distribution support in http client is pending.

Related Documentation :
https://docs.google.com/document/d/1OsRTPpU9AUNbyV-LPBG4TQFtPIcyISdrFuw3AREIy4M/edit?usp=sharing

@ao2017 ao2017 requested a review from a team October 13, 2020 20:06
@@ -12,23 +12,18 @@ message Metric {
// time in ms when metric was generated.
int64 time = 2;

// value of metric.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the docs we shouldn't remove the fields but mark them reserved

@malish8632
Copy link
Contributor

There are couple of points I've raised in regards of publishing metrics to PubSub with new serializer - not sure if the integration with Heroic consumer was ever tested. I think we should make sure it passes that test.
Also there were multiple new empty lines added across several files - could you remove them?

@ao2017
Copy link
Contributor Author

ao2017 commented Oct 21, 2020

Which test are you talking about ? Shouldn't we trust our build process.

I am sorry if I added extra lines. Where do see the empty lines ? I will revert those changes.

Making changes to a huge codebase with many components require a lot of considerations. If we can remove style preference from that equation, I think that would be a win. You may want to update the checkstyle to reflect your preference. We will catch these issues before PR submission.

@malish8632
Copy link
Contributor

If it's possible to create ffwd docker image manually and test on staging to see that Heroic consumers are capable to consume new messages before merging it, that would be great.
I understand the size of the PR. If you could just glance over this PR here in github you'll notice some additional lines.

Copy link
Contributor

@malish8632 malish8632 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to test on staging before merging. Otherwise 👍

lmuhlha and others added 2 commits October 23, 2020 12:33
@ao2017 ao2017 closed this Nov 17, 2020
@ao2017 ao2017 deleted the adele/distribution_4 branch November 17, 2020 17:22
@ao2017 ao2017 restored the adele/distribution_4 branch November 17, 2020 17:42
@ao2017
Copy link
Contributor Author

ao2017 commented Nov 17, 2020

I closed the pr by mistake

@ao2017 ao2017 reopened this Nov 17, 2020
@ao2017 ao2017 merged commit 3ab44b8 into spotify:master Nov 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants