New flag 'ingest-from' #1382

fkaleo · 2019-07-05T14:58:41Z

New flag 'ingest-from' that instructs metrictank to discard any point of a specific org id that does not belong to chunks starting after a given timestamp.
e.g. 20:67031 with a chunk span of 10 will discard all points with org id 20 and timestamps lesser than 67040

woodsaj · 2019-07-05T15:21:11Z

why limit this to only 1 orgId? Whould it not be better to just have AggMetrics use a map of orgId's to ingestAfter timstamp. eg ingestAfter map[int32]int32

Users can then pass a list of org:ts pairs via the config. eg
metrictank --ingest-after=1:1562339919,1234:1562339919,45:1562339919

fkaleo · 2019-07-08T12:11:17Z

why limit this to only 1 orgId? Whould it not be better to just have AggMetrics use a map of orgId's to ingestAfter timstamp. eg ingestAfter map[int32]int32

Users can then pass a list of org:ts pairs via the config. eg
metrictank --ingest-after=1:1562339919,1234:1562339919,45:1562339919

The only reason was the fact that we did not need it and kept the code a little simpler. We can always add it later.

fkaleo · 2019-07-08T12:15:06Z

why limit this to only 1 orgId? Whould it not be better to just have AggMetrics use a map of orgId's to ingestAfter timstamp. eg ingestAfter map[int32]int32
Users can then pass a list of org:ts pairs via the config. eg
metrictank --ingest-after=1:1562339919,1234:1562339919,45:1562339919

The only reason was the fact that we did not need it and kept the code a little simpler. We can always add it later.

Using a map is probably less code though; let me try that.

robert-milan

I think this might need to be rebased on master

mdata/aggmetric.go

fkaleo · 2019-07-31T13:11:59Z

I think this might need to be rebased on master

Done

cmd/metrictank/metrictank.go

mdata/aggmetric.go

util/flag_parsers_test.go

mdata/aggmetric.go

…-> timestamp

…elies on

…omT0'

…k boundary already

Dieterbe · 2019-08-08T21:24:48Z

mdata/aggregator_test.go

+ expected = []schema.Point{
+ {Val: 5, Ts: 120},
+ }
+ compare("simple-min-ingest-from-on-chunk-boundary", agg.minMetric, expected)


too tired now, but tomorrow i'ld like to look into this one last thing, about the timestamps going 130->120 and how that may or should not affect things.

LGTM. just need to look at one last thing tomorrow.

Thank you! Your changes look great.

Dieterbe · 2019-08-08T21:26:01Z

LGTM. just need to look at one last thing tomorrow.

mdata/aggmetric_test.go

mdata/aggregator_test.go

…nto account

Dieterbe

LGTM
Please have a close look at the last 2 commits I added.
If they make sense, merge this.
If not, let's talk.

fkaleo · 2019-08-12T09:03:35Z

LGTM
Please have a close look at the last 2 commits I added.
If they make sense, merge this.
If not, let's talk.

They make sense. I'm adding test cases to cover the last one.

… timestamp is added

Dieterbe · 2019-08-12T09:23:11Z

I already covered that in the test case i added.
if we need any more tests, it should be one that uses an actual AggMetric with aggregators on it, and confirms the right points make it in to the aggregator (points in the last aggspan before the first t0)

fkaleo · 2019-08-12T09:53:16Z

I already covered that in the test case i added.
if we need any more tests, it should be one that uses an actual AggMetric with aggregators on it, and confirms the right points make it in to the aggregator (points in the last aggspan before the first t0)

The test case added does not fail when removing the code changes.
And yep, I'm adding a test to an AggMetric and discovering that in fact more code changes need to happen to make it pass, I think.

fkaleo · 2019-08-12T11:09:27Z

Please take a look at the 2 additional tests and the change in code and let me know what you think.

mdata/aggmetric_test.go

fkaleo changed the title ~~[WIP] New flag 'ingest-after'~~ New flag 'ingest-after' Jul 16, 2019

fkaleo marked this pull request as ready for review July 16, 2019 08:50

fkaleo requested a review from robert-milan July 30, 2019 15:59

robert-milan reviewed Jul 31, 2019

View reviewed changes

mdata/aggmetric.go Outdated Show resolved Hide resolved

fkaleo force-pushed the ingest_after branch from 3f3a468 to f4235e2 Compare July 31, 2019 13:06

Dieterbe reviewed Jul 31, 2019

View reviewed changes

cmd/metrictank/metrictank.go Outdated Show resolved Hide resolved

Dieterbe reviewed Aug 7, 2019

View reviewed changes

mdata/aggmetric.go Outdated Show resolved Hide resolved

Dieterbe reviewed Aug 8, 2019

View reviewed changes

util/flag_parsers_test.go Outdated Show resolved Hide resolved

fkaleo force-pushed the ingest_after branch 3 times, most recently from 6a00142 to 3bb1d5f Compare August 8, 2019 13:01

Dieterbe reviewed Aug 8, 2019

View reviewed changes

mdata/aggmetric.go Outdated Show resolved Hide resolved

Dieterbe force-pushed the ingest_after branch from 0eaedfb to 4bdefdd Compare August 8, 2019 13:49

fkaleo changed the title ~~New flag 'ingest-after'~~ New flag 'ingest-from' Aug 8, 2019

Dieterbe force-pushed the ingest_after branch from 4bdefdd to 55246b7 Compare August 8, 2019 14:40

fkaleo added 11 commits August 8, 2019 23:42

New option 'ingest-after'

d017857

AggMetric: consider ingestAfter 0 as no-op

944db44

Fixed test

d0a0b5e

AggMetrics.ingestAfter: simplify and generalise by using a map orgId …

0f8de4c

…-> timestamp

Slight performance improvement

33b00cf

Simplified 'ingest-after' flag documentation

0f38e64

'ingest-after' flag: support comma separated pairs of org-id:timestamp

e8bb2e4

flag parser: new ParseIngestAfterFlag that MustParseIngestAfterFlag r…

bb92223

…elies on

Renamed flag 'ingest-after' into 'ingest-from'

13e2442

Simplified variable naming: 'ingestFromNextChunkT0' becomes 'ingestFr…

bc59f88

…omT0'

Changed 'ingest-from' definition to include timestamp if it is a chun…

2008776

…k boundary already

Dieterbe added 3 commits August 8, 2019 23:42

better wording

3689d6a

./scripts/dev/sync-configs.sh

1d0bebd

clean up flag parsing and testing

41fece8

Dieterbe force-pushed the ingest_after branch from 55246b7 to 41fece8 Compare August 8, 2019 20:42

simpler

ca6c43e

Dieterbe reviewed Aug 8, 2019

View reviewed changes

Dieterbe reviewed Aug 9, 2019

View reviewed changes

mdata/aggmetric_test.go Show resolved Hide resolved

Dieterbe reviewed Aug 9, 2019

View reviewed changes

mdata/aggregator_test.go Show resolved Hide resolved

fkaleo and others added 3 commits August 9, 2019 17:17

Tests: added exaplanations as to why some data points are not taken i…

3b4625e

…nto account

extra test case

2a189e0

let aggregators decide whether to keep or drop the point

16782ef

Dieterbe approved these changes Aug 9, 2019

View reviewed changes

New test case for Aggregator covering case when a point with an older…

909b87e

… timestamp is added

AggMetric: added test for GetAggregated()

2523ea2

Dieterbe reviewed Aug 12, 2019

View reviewed changes

mdata/aggmetric_test.go Outdated Show resolved Hide resolved

AggMetric.GetAggregated(): added failing test when using ingestFrom

4819abf

fkaleo force-pushed the ingest_after branch from 6c2aae4 to e9e09da Compare August 12, 2019 12:44

AggMetric test: no need to initialize empty slice

a27e9f4

fkaleo force-pushed the ingest_after branch from ad7d980 to a27e9f4 Compare August 12, 2019 13:41

fkaleo merged commit e2c64e2 into master Aug 12, 2019

fkaleo deleted the ingest_after branch August 12, 2019 14:04

Dieterbe mentioned this pull request Aug 12, 2019

when adding new point to aggregate, always see if we can flush #1425

Merged

fkaleo mentioned this pull request Oct 12, 2019

add 'ingest_after' flag to metrictank #1296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New flag 'ingest-from' #1382

New flag 'ingest-from' #1382

fkaleo commented Jul 5, 2019 •

edited

woodsaj commented Jul 5, 2019

fkaleo commented Jul 8, 2019

fkaleo commented Jul 8, 2019

robert-milan left a comment

fkaleo commented Jul 31, 2019

Dieterbe Aug 8, 2019

fkaleo Aug 8, 2019

Dieterbe commented Aug 8, 2019

Dieterbe left a comment

fkaleo commented Aug 12, 2019 •

edited

Dieterbe commented Aug 12, 2019

fkaleo commented Aug 12, 2019

fkaleo commented Aug 12, 2019

New flag 'ingest-from' #1382

New flag 'ingest-from' #1382

Conversation

fkaleo commented Jul 5, 2019 • edited

woodsaj commented Jul 5, 2019

fkaleo commented Jul 8, 2019

fkaleo commented Jul 8, 2019

robert-milan left a comment

Choose a reason for hiding this comment

fkaleo commented Jul 31, 2019

Dieterbe Aug 8, 2019

Choose a reason for hiding this comment

fkaleo Aug 8, 2019

Choose a reason for hiding this comment

Dieterbe commented Aug 8, 2019

Dieterbe left a comment

Choose a reason for hiding this comment

fkaleo commented Aug 12, 2019 • edited

Dieterbe commented Aug 12, 2019

fkaleo commented Aug 12, 2019

fkaleo commented Aug 12, 2019

fkaleo commented Jul 5, 2019 •

edited

fkaleo commented Aug 12, 2019 •

edited