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 SumObserver and UpDownSumObserver instruments #789

Merged
merged 38 commits into from
Jun 9, 2020

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Jun 8, 2020

Resolves [#755] and [#756]

Abstracts Observer instrument as a base class for all asynchronous instruments.

SumObserver and UpDownSumObserver are also cumulative, so they use the LastValueAggregator to record.
Observed values must also be non-decreasing, compared to the previous observed value for SumObserver. Observed values can be any value for UpDownSumObserver .

@lzchen lzchen requested a review from a team as a code owner June 8, 2020 19:45
@lzchen lzchen changed the title Add SumObserver instrument Add SumObserver and UpDownSumObserver instruments Jun 9, 2020
@lzchen lzchen added api Affects the API package. metrics needs reviewers PRs with this label are ready for review and needs people to review to move forward. sdk Affects the SDK package. labels Jun 9, 2020
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -189,6 +189,30 @@ def observe(self, value: ValueT, labels: Dict[str, str]) -> None:
"""


class SumObserver(Observer):
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to update the ObserverT TypeVar in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we have ObserverT = TypeVar("ObserverT", bound=Observer). I believe as long as the class used is a subclass of "Observer" this definition should be accurate?

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry you're right!

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Overall a couple minor changes, but I think the code does as expected.

Unrelated to this PR per se, but: I feel like both of these observers Sum and UpDownSum, aren't super valuable.

At the end of the day, all these things do is provide a sanity check for a developer who uses them, by providing guarantees around the value itself.

Theres edge cases here especially with things like monotonic counters that will be nasty, including:

  1. random resets back to 0 that occur due to some internal counter in the kernel resetting, or some value outside of this. SumObserver is still the right category of observer here, but you'll be violating it's constraint through no fall of your own.

In my experience these type of constraints can be handled on the consumer side, with functions or operations like nonNegativeDerivative which will show you the rate by which something is increasing.

@lzchen
Copy link
Contributor Author

lzchen commented Jun 9, 2020

@toumorokoshi
Your analysis for the usefulness between SumObserver and UpDownSumObserver makes sense. There doesn't appear to be many differences functionality wise. I believe the SumObserver is better suited for rate calculations (as you've pointed out for monotonicity), as a pseudo "promise" that all values should be non-decreasing so some meaningful insight can be derived from the non-decreasing numbers, and rates can safely be calculated. As for the edge cases you've pointed out, I don't believe any type of instrument can handle those appropriately currently.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

LGTM, some non-blocking suggestions

opentelemetry-api/CHANGELOG.md Outdated Show resolved Hide resolved
opentelemetry-sdk/CHANGELOG.md Outdated Show resolved Hide resolved
lzchen and others added 3 commits June 9, 2020 15:09
Co-authored-by: alrex <alrex.boten@gmail.com>
Co-authored-by: alrex <alrex.boten@gmail.com>
@lzchen lzchen merged commit 31e29fc into open-telemetry:master Jun 9, 2020
@lzchen lzchen deleted the sumobs branch June 9, 2020 22:53
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Jun 11, 2020
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package. metrics needs reviewers PRs with this label are ready for review and needs people to review to move forward. sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants