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

Add Sketch Aggregator #213

Merged
merged 13 commits into from
Aug 4, 2020
Merged

Conversation

ankit-bhargava
Copy link
Contributor

@ankit-bhargava ankit-bhargava commented Jul 27, 2020

This PR builds out SDK support for the Sketch Aggregator which combines updates to metric instruments in meaningful values. This aggregator in particular uses a compression scheme which preserves accuracy when distributions as severely skewed right. More information about the algorithm can be found in Datadog's paper on DDSketch. The aggregator is dependent on the Metric Instruments API (pending approval in PR # 161) and the aggregator base classes PR # 178 -- it will not compile without those files.

Also included are tests for which validate the correctness of aggregation in both single-threaded and concurrent situations.

@ankit-bhargava ankit-bhargava requested a review from a team July 27, 2020 05:57
@ankit-bhargava ankit-bhargava changed the title sketch aggregator with tests Add Sketch Aggregator Jul 27, 2020
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #213 into master will increase coverage by 0.87%.
The diff coverage is 97.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   92.19%   93.07%   +0.87%     
==========================================
  Files         117      124       +7     
  Lines        4038     4619     +581     
==========================================
+ Hits         3723     4299     +576     
- Misses        315      320       +5     
Impacted Files Coverage Δ
api/test/nostd/shared_ptr_test.cc 100.00% <ø> (ø)
ext/test/zpages/tracez_processor_test.cc 98.68% <ø> (ø)
...clude/opentelemetry/sdk/common/atomic_unique_ptr.h 100.00% <ø> (ø)
...include/opentelemetry/sdk/common/circular_buffer.h 100.00% <ø> (ø)
...e/opentelemetry/sdk/common/circular_buffer_range.h 100.00% <ø> (ø)
sdk/test/common/atomic_unique_ptr_test.cc 100.00% <ø> (ø)
sdk/test/common/circular_buffer_range_test.cc 100.00% <ø> (ø)
sdk/test/common/circular_buffer_test.cc 100.00% <ø> (ø)
sdk/src/trace/batch_span_processor.cc 93.67% <93.67%> (ø)
ext/test/zpages/tracez_data_aggregator_test.cc 97.21% <97.21%> (ø)
... and 20 more

#include "opentelemetry/metrics/instrument.h"
#include "opentelemetry/version.h"
#include "opentelemetry/sdk/metrics/aggregator/aggregator.h"
#include <vector>
Copy link
Member

Choose a reason for hiding this comment

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

probably need to fix the order of the include

@reyang
Copy link
Member

reyang commented Jul 30, 2020

I don't have enough knowledge to review this PR, please see if @jmacd could help.

@reyang reyang added the help wanted Good for taking. Extra help will be provided by maintainers label Jul 30, 2020
@jmacd
Copy link

jmacd commented Jul 30, 2020

It's nice to see a new implementation of DDSketch, and I'd be happy to take a closer look at this PR some time after today.

There is an issue we're having with Sketch aggregators in across OTel, which I didn't realize when I first added the DDSketch aggregator in OTel-Go. This aggregator is definitely useful for computing Summary values, i.e., quantile information. But it's not something we know how to export over OTLP because AFAIK DataDog has not proposed a serialization format.

I'm not sure where to go with this information. Any sketch aggregator that enters OTLP will need an encoding that will have to be specified. I suppose we can literally encode the state of the DDSketch buckets and that's all we need. Have you looked into serializing and deserializing this structure, as we might need to do in the collector?

@ankit-bhargava
Copy link
Contributor Author

@jmacd We were thinking of either exporting the state of the buckets as you mentioned which would be similar to the way a histogram would be serialized or allowing the user to specify certain quantiles of interest when initializing the aggregator. These quantiles would then be printed as opposed to the entire data structure.

@jmacd
Copy link

jmacd commented Jul 30, 2020

Sounds good. I'll be happy to review the implementation, but can't do this today.

@jmacd
Copy link

jmacd commented Aug 4, 2020

This looks good!

I would reserve the generic name "Sketch" to describe this category of Aggregator, and use "DDSketch" or "ddsketch" file and directory names for this implementation, to be more specific.

It will be good to start thinking about how to represent DDSketch inside OTLP, since we have several implementations available.

@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Aug 4, 2020
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang
Copy link
Member

reyang commented Aug 4, 2020

@ankit-bhargava there is a test failure related to this PR, please investigate.

@ankit-bhargava
Copy link
Contributor Author

ankit-bhargava commented Aug 4, 2020

@reyang Looking into it now, thanks. Seems like a call to vector.size() is returning a negative value, not entirely sure how that's possible.

@ankit-bhargava
Copy link
Contributor Author

@reyang Cleared up all CI issues and rebased, should be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Good for taking. Extra help will be provided by maintainers pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants