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

chore: adding metric observable to be able to support async update #964

Merged
merged 8 commits into from
Apr 22, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Apr 13, 2020

Which problem is this PR solving?

Short description of the changes

  • It adds support to be able to update the metric observer value in async operation

@obecny obecny self-assigned this Apr 13, 2020
@obecny obecny added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 13, 2020
@codecov-io
Copy link

codecov-io commented Apr 13, 2020

Codecov Report

Merging #964 into master will decrease coverage by 0.06%.
The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           master     #964      +/-   ##
==========================================
- Coverage   95.02%   94.95%   -0.07%     
==========================================
  Files         209      210       +1     
  Lines        8541     8569      +28     
  Branches      766      770       +4     
==========================================
+ Hits         8116     8137      +21     
- Misses        425      432       +7     
Impacted Files Coverage Δ
...ages/opentelemetry-metrics/src/MetricObservable.ts 53.33% <53.33%> (ø)
packages/opentelemetry-metrics/src/Metric.ts 96.87% <92.85%> (+0.20%) ⬆️
...ckages/opentelemetry-metrics/src/ObserverResult.ts 100.00% <100.00%> (ø)
packages/opentelemetry-metrics/test/Meter.test.ts 99.35% <100.00%> (+0.01%) ⬆️

packages/opentelemetry-api/src/metrics/MetricObservable.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/MetricObservable.ts Outdated Show resolved Hide resolved
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This looks good thanks for this

@dyladan dyladan added API enhancement New feature or request labels Apr 13, 2020

next(value: number) {
for (const subscriber of this._subscribers) {
subscriber(value);
Copy link
Member

Choose a reason for hiding this comment

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

Can we use setImmediate(subscriber, value) to avoid calling subscribers synchronously

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do think it would be a problem if we call them synchronously. The common pattern for such things is to make it synchronous. Also if user is calling "next" I would assume that this is the moment it should be updated, including the time as well, using setImmediate the time won't be accurate. I also think that this might cause some unexpected behaviour, if for any reason you would like to also call "collect" after you update values. I would be rather against that.

Copy link
Member

Choose a reason for hiding this comment

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

Just to avoid blocking the event loop for too long if there are many subscribers to update, or if the subscribers take too long to process a value. A user may also subscribe to updates, not just us, and the user is unlikely to know their callback is in a performance sensitive context

Copy link
Member Author

@obecny obecny Apr 15, 2020

Choose a reason for hiding this comment

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

The "hidden" async operation will prevent user from calling collect immediately after updating the values, and time for updating the value will also be different then moment of calling next. Besides that "blocking loop" would last as long as callback in "standard way" anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mayurkale22 @vmarchaud whats your thoughts on that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@vmarchaud setImmediate will delay update - it won't be updated asap as spec says

Copy link
Member

Choose a reason for hiding this comment

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

I don't find any requirement on specific time requirement, could you give a link please ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@vmarchaud you wrote the spec state that the update path should be as fast as possible so setImmediate won't update asap but it will update in next cycle

Copy link
Member

Choose a reason for hiding this comment

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

I see, the whole spec state:

Metric updates made via a bound instrument, when used with an Aggregator defined by simple atomic operations, should follow a very short code path

I interpret short code path from a performance requirement and not as a time requirement. So using setImmediate would be a better solution for me

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm strongly against it but if majority want's setImmediate with it's all consequences fine for me, @mayurkale22 you also vote for setImmediate ?

@obecny
Copy link
Member Author

obecny commented Apr 20, 2020

up ^^

@andrewhsu
Copy link
Member

andrewhsu commented Apr 22, 2020

From the javascript sig mtg today talking with @dyladan , sounds like having setImmediate is not a blocker for this PR to merge, so just need to resolve conflict to bring this one in.

If setImmediate is super desired afterwards, sounds like can be a followup PR.

@dyladan dyladan merged commit 998f3f6 into open-telemetry:master Apr 22, 2020
@obecny obecny deleted the metric_observable branch July 8, 2020 12:16
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
…en-telemetry#964)

* chore: adding metric observable to be able to support async update

* chore: reviews

* chore: reviews

Co-authored-by: Daniel Dyla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metric Observer - change from callback to Observable
6 participants