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

Refactor metrics instrument #2297

Merged
merged 6 commits into from
Dec 14, 2021
Merged

Refactor metrics instrument #2297

merged 6 commits into from
Dec 14, 2021

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Nov 27, 2021 •
edited
Loading

Refactors instruments

  1. Moves the handling of callable and generator callbacks for asynchronous instruments to the SDK
  2. Adds default aggregations to instruments
  3. Adds test cases

@ocelotl ocelotl added Skip Changelog PRs that do not require a CHANGELOG.md entry Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary labels Nov 27, 2021
@ocelotl ocelotl requested a review from aabmass November 27, 2021 02:19
@ocelotl ocelotl self-assigned this Nov 27, 2021
@ocelotl ocelotl requested a review from a team November 27, 2021 02:19
@ocelotl ocelotl marked this pull request as draft November 27, 2021 02:19
@ocelotl ocelotl mentioned this pull request Nov 27, 2021
Closed
@ocelotl ocelotl force-pushed the issue_2294 branch 6 times, most recently from 7209a5c to 29f5752 Compare November 29, 2021 03:04
This was referenced Dec 2, 2021
Merged
Merged
@ocelotl ocelotl marked this pull request as ready for review December 6, 2021 19:44
@ocelotl ocelotl added the metrics label Dec 7, 2021
@aabmass
Copy link
Member

aabmass commented Dec 7, 2021 •
edited
Loading

Review #2296 first

Looks like this PR is stacked on that other PR. Can you make a more descriptive title and description, and put this back in draft until the other one is merged. It's very hard to review when all the changes are visible globally.

@ocelotl ocelotl marked this pull request as draft December 8, 2021 21:13
@ocelotl
Copy link
Contributor Author

ocelotl commented Dec 8, 2021

Review #2296 first

Looks like this PR is stacked on that other PR. Can you make a more descriptive title and description, and put this back in draft until the other one is merged. It's very hard to review when all the changes are visible globally.

Sure, marking this one as draft and rebasing ✌️

@ocelotl ocelotl force-pushed the issue_2294 branch 2 times, most recently from 613dafd to 33d6ac2 Compare December 8, 2021 22:37
@ocelotl ocelotl marked this pull request as ready for review December 8, 2021 22:37
@ocelotl
Copy link
Contributor Author

ocelotl commented Dec 8, 2021

Review #2296 first

Looks like this PR is stacked on that other PR. Can you make a more descriptive title and description, and put this back in draft until the other one is merged. It's very hard to review when all the changes are visible globally.

Sure, marking this one as draft and rebasing v

Rebased, marking as ready for review 😎

aabmass
aabmass reviewed Dec 9, 2021
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM just a few comments

opentelemetry-api/src/opentelemetry/_metrics/instrument.py Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py Show resolved Hide resolved
opentelemetry-sdk/tests/metrics/integration_test/test_cpu_time.py Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_metrics/instrument.py Outdated Show resolved Hide resolved
aabmass
aabmass approved these changes Dec 13, 2021
View reviewed changes
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 once the open discussions are resolved

@NathanielRN NathanielRN changed the title Refactor instrument Refactor metrics instrument Dec 13, 2021
@ocelotl ocelotl mentioned this pull request Dec 14, 2021
Closed
@ocelotl ocelotl merged commit 636262d into open-telemetry:main Dec 14, 2021
@ocelotl ocelotl deleted the issue_2294 branch December 14, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen left review comments

@srikanthccv srikanthccv srikanthccv approved these changes

@aabmass aabmass aabmass approved these changes

@codeboten codeboten codeboten approved these changes

Assignees

@ocelotl ocelotl

Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary metrics Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ocelotl @aabmass @codeboten @lzchen @srikanthccv