By default downsample all metrics for aggregated cluster namespaces #890
Conversation
…egated cluster namespace
src/query/server/server.go
Outdated
| if attrs.MetricsType == storage.AggregatedMetricsType { | ||
| downsampleOpts, err := opts.DownsampleOptions() | ||
| if err != nil { | ||
| logger.Fatal("unable to resolve downsample options", |
There was a problem hiding this comment.
nit: could you bubble the error up and fatal in main/run
There was a problem hiding this comment.
Sure thing.
docs/integrations/prometheus.md
Outdated
| @@ -32,7 +32,8 @@ clusters: | |||
| # We created a namespace called "default" and had set it to retention "48h". | |||
| - namespace: default | |||
| retention: 48h | |||
| storageMetricsType: unaggregated | |||
| resolution: 1m | |||
There was a problem hiding this comment.
maybe add a little blurb that this is optional and what it does
There was a problem hiding this comment.
Hm, this was an oopsy, I didn't mean to make this aggregated hah.
| @@ -63,7 +63,8 @@ func NewDownsampler( | |||
|
|
|||
| func (d *downsampler) NewMetricsAppender() MetricsAppender { | |||
| return newMetricsAppender(metricsAppenderOptions{ | |||
| agg: d.agg.aggregator, | |||
| agg: d.agg.aggregator, | |||
There was a problem hiding this comment.
weird that this didn't align, did go fmt break on this?
There was a problem hiding this comment.
Honestly, no idea. gofmt was the tool that aligned it like this. Let's just leave it for now since that how it wants to align it?
Codecov Report
@@ Coverage Diff @@
## master #890 +/- ##
==========================================
- Coverage 78.34% 78.33% -0.01%
==========================================
Files 396 396
Lines 33591 33654 +63
==========================================
+ Hits 26316 26363 +47
- Misses 5468 5484 +16
Partials 1807 1807
Continue to review full report at Codecov.
|
| @@ -32,7 +32,7 @@ clusters: | |||
| # We created a namespace called "default" and had set it to retention "48h". | |||
| - namespace: default | |||
| retention: 48h | |||
| storageMetricsType: unaggregated | |||
| type: unaggregated | |||
There was a problem hiding this comment.
is this going to be b/w compatible? main concern is if skipping specifying this field be the same as unaggregated?
There was a problem hiding this comment.
Hm, I'll add a support for both to be used in a backwards compatible way. Ugh, good catch.
richardartoul
force-pushed
the
master
branch
from
2883443
to
2cf04be
Compare
src/query/storage/local/config.go
Outdated
| Retention time.Duration `yaml:"retention" validate:"nonzero"` | ||
| Resolution time.Duration `yaml:"resolution" validate:"min=0"` | ||
| Namespace string `yaml:"namespace"` | ||
| Type storage.MetricsType `yaml:"type"` |
There was a problem hiding this comment.
re: our conversation about deprecated, looks like there is a convention for this in Go already: https://go-review.googlesource.com/c/blog/+/18956/2/content/godoc-documenting-go-code.article
|
Did not mean to "dismiss stale review" and will merge this based on @prateek's sign off. |
robskillington
deleted the
r/add-aggregated-namespace-auto-mapping-rules
branch
robskillington
changed the title
No description provided.