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 Graphite ingestion #1310

Merged
merged 70 commits into from
Jan 25, 2019
Merged

Add Graphite ingestion #1310

merged 70 commits into from
Jan 25, 2019

Conversation

richardartoul
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #1310 into master will decrease coverage by <.1%.
The diff coverage is 50.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1310     +/-   ##
========================================
- Coverage    70.7%   70.7%   -0.1%     
========================================
  Files         770     770             
  Lines       64716   64747     +31     
========================================
+ Hits        45776   45783      +7     
- Misses      15978   15997     +19     
- Partials     2962    2967      +5
Flag Coverage Δ
#aggregator 80.9% <ø> (ø) ⬆️
#cluster 85.6% <ø> (ø) ⬆️
#collector 78.4% <ø> (ø) ⬆️
#dbnode 80.8% <33.3%> (-0.1%) ⬇️
#m3em 73.2% <ø> (ø) ⬆️
#m3ninx 74.7% <ø> (ø) ⬆️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.8% <0%> (-0.1%) ⬇️
#msg 75% <ø> (+0.1%) ⬆️
#query 61.9% <54.9%> (ø) ⬆️
#x 75.9% <ø> (ø) ⬆️

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 4cc991b...29c5a88. Read the comment docs.

ctx context.Context,
iter DownsampleAndWriteIter,
) error {
if d.downsampler != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

start goroutines first then do this then wait

type: aggregated
retention: 10h
resolution: 15s
- namespace: unagg
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add a TODO to remove this once unaggregated is no longer mandatory?

@richardartoul richardartoul changed the title [WIP] - Add Graphite ingestion Add Graphite ingestion Jan 25, 2019
// Limits specifies limits on per-query resource usage.
Limits LimitsConfiguration `yaml:"limits"`
}

// CarbonConfiguration returns the carbon configuration.
func (c *Configuration) CarbonConfiguration() *CarbonConfiguration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this return CarbonConfiguration instead of *CarbonConfiguration? That way it's always not nil when used as a local var returned from this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ended up not needing this accessor at all. Made everything non-point and then used the getter pattern you described above.

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@richardartoul richardartoul merged commit 8a29241 into master Jan 25, 2019
robskillington pushed a commit that referenced this pull request Jan 28, 2019
@justinjc justinjc deleted the ra/carbon-ingest branch June 17, 2019 21:56
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.

4 participants