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

[tests] Add In-Process Aggregator (Start Only) #3795

Merged
merged 3 commits into from
Oct 1, 2021
Merged

Conversation

fishie9
Copy link
Collaborator

@fishie9 fishie9 commented Sep 30, 2021

What this PR does / why we need it:

This PR adds resources.Aggregator interface with a few basic functions.
It also adds an in-process implementation of the new interface which will start in process.

More PRs to follow:

  • Support of starting multiple aggregator instances.
  • Implementation of the functions in resources.Aggregator.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:


Does this PR require updating code package or user-facing documentation?:


@CLAassistant
Copy link

CLAassistant commented Sep 30, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Siyu Yang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@fishie9 fishie9 force-pushed the siyu/aggtest branch 2 times, most recently from 8b2f151 to 7c36a71 Compare September 30, 2021 01:17
@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #3795 (647570c) into master (51817b0) will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3795     +/-   ##
========================================
+ Coverage    56.8%   57.0%   +0.1%     
========================================
  Files         552     552             
  Lines       63064   63064             
========================================
+ Hits        35856   35965    +109     
+ Misses      24008   23905    -103     
+ Partials     3200    3194      -6     
Flag Coverage Δ
aggregator 63.3% <ø> (-0.1%) ⬇️
cluster ∅ <ø> (∅)
collector 58.4% <ø> (ø)
dbnode 60.7% <ø> (+0.2%) ⬆️
m3em 46.4% <ø> (ø)
metrics 19.7% <ø> (ø)
msg 74.3% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51817b0...647570c. Read the comment docs.

@fishie9 fishie9 requested a review from nbroyles September 30, 2021 01:35
@@ -26,13 +26,32 @@ import (
"github.com/m3db/m3/src/x/log"
)

var (
defaultMetricsSanitization = instrument.PrometheusMetricSanitization
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do these defaults come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied from the dbnode test.
I reverted this change and added the metrics config in the test yaml again.
I can work on the default values later in separate PRs.

@@ -89,7 +89,7 @@ type AggregatorConfiguration struct {
GaugePrefix *string `yaml:"gaugePrefix"`

// Stream configuration for computing quantiles.
Stream streamConfiguration `yaml:"stream"`
Stream *streamConfiguration `yaml:"stream"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this nillable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was trying to cut the unused part in the test config yaml.
Just changed it back and added a minimal stream configuration in the test.

@@ -0,0 +1,278 @@
//go:build integration_v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's get rid of one of these. Only need one tag here.

Siyu Yang added 3 commits October 1, 2021 12:51

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@fishie9 fishie9 merged commit 0225ecb into master Oct 1, 2021
@fishie9 fishie9 deleted the siyu/aggtest branch October 1, 2021 19:53
Antanukas pushed a commit that referenced this pull request Oct 5, 2021
This commit adds resources.Aggregator interface with a few basic functions.
It also adds an in-process implementation of the new interface which will start in process.
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.

None yet

3 participants