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

feat/add histogram aggregator in defaultAggregator #226

Merged
merged 20 commits into from
May 30, 2022

Conversation

NeJan2020
Copy link
Collaborator

Because we have started to use a stand-alone aggregator instead of OTLP-SDK, some functions that were originally implemented by the OTLP-SDK aggregator are gradually being moved to the stand-alone aggregator. Histogram aggregation is one of them and will be needed in standard version of kindling

During this process, I ran into some problems and decided to make some modifications to the model.Gauge structure

Reasons for changing the model of Gauge

  1. If the aggregator needs to support Histogram output, the ExplicitBoundaries and BucketCounts information must be included in the output. The current structure is not suitable for storing this kind of information.
  2. This modification has little damage to the structure and is less difficult to modify, but it improves the scalability of the structure.

Reasons for keeping the model of Gauge

  1. As the underlying data structure of the pipeline, Gauge is used too widely. Many components need to be adjusted
  2. The original Gauge structure has simple functions and is easy to understand. Not all developers have the basic knowledge of Histogram type data.

Because implementing the Histogram based on modifying the Gauge structure is far simpler than not modifying it. I provided this PR which provides a simple implementation to illustrate the impact of Gauge changes on other components. At the same time, it has been able to support the aggregation of Histogram type data.

Description

  1. BREAKCHANGE: Change the model of Gauge
  2. Add a new aggregator value named histogram in the default aggregator

Motivation and Context

The existing performance of the aggregator based on the OTLP-SDK cannot support our needs, and our new aggregate processor can not support Histogram.

How Has This Been Tested?

cd collector && go test -v github.com/Kindling-project/kindling/collector/pkg/aggregator/defaultaggregator
=== RUN Test_aggValues_sum
--- PASS: Test_aggValues_sum (0.00s)
=== RUN Test_aggValues_sum_histogram
--- PASS: Test_aggValues_sum_histogram (0.00s)
=== RUN Test_aggValues_max
--- PASS: Test_aggValues_max (0.01s)
=== RUN Test_aggValues_max_histogram
--- PASS: Test_aggValues_max_histogram (0.00s)
=== RUN Test_aggValues_avg
--- PASS: Test_aggValues_avg (0.01s)
=== RUN Test_aggValues_avg_histogram
--- PASS: Test_aggValues_avg_histogram (0.01s)
=== RUN Test_aggValues_last
--- PASS: Test_aggValues_last (0.00s)
=== RUN Test_aggValues_last_histogram
--- PASS: Test_aggValues_last_histogram (0.00s)
=== RUN Test_aggValues_count
--- PASS: Test_aggValues_count (0.00s)
=== RUN Test_aggValues_count_histogram
--- PASS: Test_aggValues_count_histogram (0.00s)
=== RUN Test_aggValues_histogram
--- PASS: Test_aggValues_histogram (0.01s)
=== RUN Test_aggValues_histogram_histogram
--- PASS: Test_aggValues_histogram_histogram (0.01s)
=== RUN Test_defaultValuesMap_sum
--- PASS: Test_defaultValuesMap_sum (0.00s)
=== RUN Test_defaultValuesMap_avg
--- PASS: Test_defaultValuesMap_avg (0.00s)
=== RUN Test_defaultValuesMap_max
--- PASS: Test_defaultValuesMap_max (0.00s)
=== RUN Test_defaultValuesMap_lastValue
--- PASS: Test_defaultValuesMap_lastValue (0.00s)
=== RUN Test_defaultValuesMap_countValue
--- PASS: Test_defaultValuesMap_countValue (0.00s)
=== RUN Test_defaultValuesMap_histogramValue
--- PASS: Test_defaultValuesMap_histogramValue (0.00s)
=== RUN TestConcurrentAggregator
--- PASS: TestConcurrentAggregator (0.56s)
=== RUN TestRecord
--- PASS: TestRecord (0.00s)
PASS

NeJan2020 added 6 commits May 17, 2022 16:11
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
@NeJan2020 NeJan2020 requested a review from dxsup May 18, 2022 04:00
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
@NeJan2020
Copy link
Collaborator Author

The raw struct of Gauge is

type Gauge struct {
	Name  string
	Value int64
}

The new struct of Gauge is

type Gauge struct {
	Name string
	//	Data can be assigned by:
	//	Int
	//	Histogram
	Data isGaugeData
}

type Int struct {
	Value int64
}

type Histogram struct {
	Sum                int64
	Count              uint64
	ExplicitBoundaries []int64
	BucketCounts       []uint64
}

type isGaugeData interface {
	isGaugeData()
}

func (*Gauge_Int) isGaugeData()       {}
func (*Gauge_Histogram) isGaugeData() {}

type Gauge_Int struct {
	Int *Int
}

type Gauge_Histogram struct {
	Histogram *Histogram
}

The implementation details of this structure refer to the oneof implementation of the protocol in golang

NeJan2020 added 4 commits May 19, 2022 18:07
1. add a simple prometheus-exporter to export histogram data;
2. add a CumulativeAggregator to store data in exporter

Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
1. rename `GaugeGroup` to `DataGroup`
2. rename `Gauge` to `Metric`

Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
NeJan2020 added 5 commits May 26, 2022 16:59
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
@NeJan2020 NeJan2020 force-pushed the fb/add_histogram_aggregator branch from d1e8462 to f5a6075 Compare May 26, 2022 12:48
NeJan2020 added 3 commits May 27, 2022 11:03
…c in constvalues

Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
@@ -6,20 +6,20 @@ import (
"sync"
)

var otelexporterGaugegroupsReceivedTotal = "kindling_telemetry_otelexporter_gaugegroups_received_total"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I changed the self-telemetry metric, will this influence other components?

Copy link
Member

Choose a reason for hiding this comment

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

No.

Signed-off-by: niejiangang <niejiangang@harmonycloud.cn>
@dxsup dxsup merged commit 63bd892 into KindlingProject:main May 30, 2022
@NeJan2020 NeJan2020 deleted the fb/add_histogram_aggregator branch June 28, 2022 08:17
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.

2 participants