search

spotify / ffwd

a flexible metric forwarding agent
https://spotify.github.io/ffwd/
Apache License 2.0
79 stars 33 forks source link

Add distribution support to ffwd agent modules #221

Closed ao2017 closed 3 years ago

ao2017 commented 3 years ago

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

malish8632 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

ao2017 commented 3 years ago

I closed the pr by mistake