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

Metrics: add possibility to release handles and observer instruments #435

Merged
merged 5 commits into from
Mar 17, 2020

Conversation

mauriciovasquezbernal
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal commented Feb 20, 2020

Fixes #399.

This commit implements a solution for releasing instrument handles and observers.

For the handles it is based on a ref count that is increased each time the
handled is acquired, when the ref count reaches 0 the handle is removed on
collection time. The direct call convention is updated to release the handle
after it has been updated.

The observer instrument is only updated on collection time, so it can be removed
as soon as the user request to do so.

@codecov-io
Copy link

codecov-io commented Feb 20, 2020

Codecov Report

Merging #435 into master will increase coverage by 0.14%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
+ Coverage   89.33%   89.48%   +0.14%     
==========================================
  Files          43       43              
  Lines        2176     2215      +39     
  Branches      248      250       +2     
==========================================
+ Hits         1944     1982      +38     
  Misses        161      161              
- Partials       71       72       +1     
Impacted Files Coverage Δ
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py 94.87% <90.00%> (+0.46%) ⬆️
...elemetry-api/src/opentelemetry/metrics/__init__.py 93.50% <100.00%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bfc48b...e77a3c3. Read the comment docs.

@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/release-handle branch 4 times, most recently from f5ba16e to 100174b Compare March 3, 2020 12:45
This commit implements a solution for releasing instrument handles and observers.

For the handles it is based on a ref count that is increased each time the
handled is acquired, when the ref count reaches 0 the handle is removed on
collection time.  The direct call convention is updated to release the handle
after it has been updated.

The observer instrument is only updated on collection time, so it can be removed
as soon as the user request to do so.
@mauriciovasquezbernal mauriciovasquezbernal marked this pull request as ready for review March 3, 2020 13:04
@mauriciovasquezbernal mauriciovasquezbernal requested a review from a team March 3, 2020 13:04
@mauriciovasquezbernal mauriciovasquezbernal added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Mar 9, 2020
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

@lzchen lzchen mentioned this pull request Mar 9, 2020
7 tasks
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM. I think this is likely to change with the removal of labelsets or the addition of metrics views, but no reason to hold this PR up now.

@c24t c24t merged commit 4e551ba into open-telemetry:master Mar 17, 2020
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/release-handle branch April 14, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release handle/bound instrument in metrics from memory upon direct metric calling convention
4 participants