Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Record a Start Time Per Time Series within a View #1220

Merged
merged 8 commits into from
Jul 14, 2020

Conversation

ian-mi
Copy link
Contributor

@ian-mi ian-mi commented Jul 10, 2020

Currently all time series within a view will use a single start time recorded when the view is registered. Consequently if a time series is first recorded long after the view is registered the first point will be misattributed to a potentially very long time interval. Instead record a separate start time per time series within AggregationData that is recorded when data for that time series is first recorded. This start time is used when converting rows to TimeSeries and may also be used directly by stats exporters.

@ian-mi ian-mi requested review from rakyll, rghetia and a team as code owners July 10, 2020 03:59
@ian-mi ian-mi changed the title Record a Start Time Per Time Series Record a Start Time Per Time Series within a View Jul 10, 2020
@ian-mi
Copy link
Contributor Author

ian-mi commented Jul 13, 2020

@nilebox

Copy link
Collaborator

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 324 to 329
case *view.CountData:
data.Start = time.Time{}
case *view.SumData:
data.Start = time.Time{}
case *view.DistributionData:
data.Start = time.Time{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

Suggested change
case *view.CountData:
data.Start = time.Time{}
case *view.SumData:
data.Start = time.Time{}
case *view.DistributionData:
data.Start = time.Time{}
case *view.CountData, *view.SumData, *view.DistributionData:
data.Start = time.Time{}

(similar for other tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work because go must resolve data to a concrete type in order to reference the Start field.

Copy link

@nilebox nilebox left a comment

Choose a reason for hiding this comment

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

Overall the idea looks good, just please cleanup the tests from duplicate code

stats/view/aggregation_data.go Outdated Show resolved Hide resolved
stats/view/worker.go Show resolved Hide resolved
plugin/ocgrpc/client_stats_handler_test.go Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants