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.

Sign up for GitHub

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

Jump to bottom

Implement UpDownCounter instrument #796

Merged
merged 9 commits into from
Jun 10, 2020
Merged

Implement UpDownCounter instrument #796

merged 9 commits into from
Jun 10, 2020

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Jun 9, 2020

Closes #754. Very similar to Counter, I just copied the classes and renamed.

Things that could be implemented differently:

  • UpDownCounter could subclass Counter, but I think that is a confusing way to do it.
  • Instead of using BoundCounter, I could copy it to a new class BoundUpDownCounter.
  • Instead of using CounterAggregator, I could create a UpDownCounterAggregator. I think it would make more sense to rename CounterAggregator -> SumAggregator and use it in both. Then SumAggregator can be used in the views API.

Let me know what you think!

@aabmass aabmass requested a review from a team June 9, 2020 20:18
ocelotl
ocelotl reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just a question regarding a test

opentelemetry-api/tests/metrics/test_metrics.py Show resolved Hide resolved
lzchen
lzchen requested changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Using SumAggregator for both makes sense to me. You'd probably need to create BoundUpDownCounter if you are going to add additional validation.

@aabmass aabmass requested a review from lzchen June 10, 2020 19:29
lzchen
lzchen approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM. Just a quick nit and wondering if you are going to rename the SumAggregator.

@aabmass
Copy link
Member Author

aabmass commented Jun 10, 2020 •
edited
Loading

Sure, I'll rename it in this PR!

EDIT: actually I'll do it in another PR to keep the noise down on this change

@codeboten codeboten merged commit 94aabcb into open-telemetry:master Jun 10, 2020
@aabmass aabmass deleted the up-down-counter-754 branch June 11, 2020 00:37
This was referenced Jun 11, 2020
Closed
Merged
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Jun 11, 2020
@aabmass
api/sdk: Implement UpDownCounter instrument (open-telemetry#796)
b750ad7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@ocelotl ocelotl ocelotl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the UpDownCounter instrument
6 participants
@aabmass @ocelotl @lzchen @codeboten @cnnradams @toumorokoshi