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

Metrics: Add specs on Cumulative APIs #254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mayurkale22
Copy link
Member

Fixes #241

@jkohen
Copy link

jkohen commented Mar 28, 2019

/cc @sumeer

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Question: how do we define and deal with start time with cumulative APIs? Do we set the start time when creating a cumulative and reset it to the time when reset is called?

metrics/Cumulative.md Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member Author

Question: how do we define and deal with start time with cumulative APIs? Do we set the start time when creating a cumulative and reset it to the time when reset is called?

In a time series, cumulative metric should have the same start time (set when creating a cumulative) and increasing end times, until an event resets then the start time should also be reset and we should use new start time for the following points.

I will update the specs with this info. Thanks

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

* `labelValues`: the list of label values.
* Clear all time series from the cumulative metric i.e. References to all previous point objects are invalid (not part of the metric).

In a time series, cumulative metric should have the same start time (set when creating a cumulative) and increasing end times, until an event resets then the start time should also be reset and we should use new start time for the following points.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the function registered returns a value that is lower than before (due to reset or some other reason) without application calling reset? Do we

  1. reset the start time whenever we detect that new value is not higher than the old value. OR
  2. continue to report old value.

I think we should do (1).

* `resource`: an optional associated monitored resource information.
* Add a new time series with label values, which returns a `Point` (which is part of the `TimeSeries`). Each point represents an instantaneous measurement of a cumulative value. Each Cumulative Metric has one or more time series for a single metric.
* `labelValues`: the list of label values. The number of label values must be the same to that of the label keys.
* The `Point` class should provide functionalities to manually increment/reset values. Example: `inc()` bye one, `inc(long times)` a specific number of times at once, `reset()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We need set() that does conditional sets the value if the old value is lower.
inc() and inc(long times) both may not be needed. For Go it would require two functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Chatted with @rghetia and @bogdandrutu offline, we agreed that a Set API for Cumulative is not needed. Users should be able to use the derived cumulative API for aggregated metrics rather than creating their own timer.

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.

5 participants