Batch observer #1137

obecny · 2020-06-03T17:26:19Z

Which problem is this PR solving?

Fixes Implement BatchObserver #1118
Fixes Rename Observer to ValueObserver #1113

Short description of the changes

It adds a BatchObserver to support observing multiple instruments in one callback.
Observer refactoring
Created separate files for each of the metric
Renamed Observer to ValueObserver
Renamed aggregators

dyladan · 2020-06-03T17:29:58Z

Does this also contain the changes we talked about for the non-batch observer?

codecov · 2020-06-03T17:30:21Z

Codecov Report

Merging #1137 into master will decrease coverage by 0.37%.
The diff coverage is 90.44%.

@@            Coverage Diff             @@
##           master    #1137      +/-   ##
==========================================
- Coverage   93.29%   92.92%   -0.38%     
==========================================
  Files         126      131       +5     
  Lines        3597     3674      +77     
  Branches      724      742      +18     
==========================================
+ Hits         3356     3414      +58     
- Misses        241      260      +19

Impacted Files	Coverage Δ
packages/opentelemetry-api/src/metrics/Metric.ts	`100.00% <ø> (ø)`
...s/opentelemetry-metrics/src/UpDownCounterMetric.ts	`100.00% <ø> (ø)`
packages/opentelemetry-metrics/src/export/types.ts	`100.00% <ø> (ø)`
...ackages/opentelemetry-api/src/metrics/NoopMeter.ts	`66.66% <46.15%> (-3.11%)`	⬇️
...pentelemetry-exporter-prometheus/src/prometheus.ts	`87.87% <70.00%> (-3.52%)`	⬇️
...ckages/opentelemetry-metrics/src/export/Batcher.ts	`91.30% <87.50%> (-8.70%)`	⬇️
packages/opentelemetry-metrics/src/Meter.ts	`92.68% <90.90%> (-1.07%)`	⬇️
packages/opentelemetry-metrics/src/Metric.ts	`97.05% <92.30%> (-2.95%)`	⬇️
...s/opentelemetry-metrics/src/BatchObserverMetric.ts	`93.10% <93.10%> (ø)`
...es/opentelemetry-metrics/src/BaseObserverMetric.ts	`100.00% <100.00%> (ø)`
... and 17 more

obecny · 2020-06-03T17:31:11Z

Does this also contain the changes we talked about for the non-batch observer?

Yes - it has both of them

mayurkale22

This PR is very big, easy to miss the things. Will take a look again.

examples/metrics/metrics/observer.js

packages/opentelemetry-api/src/metrics/BatchObserverResult.ts

packages/opentelemetry-metrics/src/Meter.ts

dyladan

Didn't have a lot of time to look at this today. It's on my list for first thing tomorrow.

examples/metrics/metrics/observer.js

packages/opentelemetry-api/src/metrics/NoopMeter.ts

obecny · 2020-06-04T12:28:58Z

This PR is very big, easy to miss the things. Will take a look again.

I have splited the code from Meter into separate files as Meter was growing bigger and bigger (different classes in one file)

markwolff

Overall LGTM, but I'll take another closer look

markwolff · 2020-06-17T22:40:06Z

packages/opentelemetry-metrics/src/BaseObserverMetric.ts

+    resource: Resource,
+    metricKind: MetricKind,


nit: reorder these two to match Metric ctor

they already match the other metric like CounterMetric, ValueRecorderMetric etc.

markwolff · 2020-06-17T22:52:17Z

packages/opentelemetry-metrics/src/Metric.ts

-      description: this._options.description,
-      unit: this._options.unit,
+      description: this._options.description || '',
+      unit: this._options.unit || '1',


It looks like this was default value previously existed here, but what does '1' mean for a unit? From the spec it seems like just a normal unit of measure, e.g. GBs or whatever. Am I misinterpreting the unit descriptor?

this is what our ~~spec~~ api says and that's why I added here

https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-api/src/metrics/Metric.ts#L37
@mayurkale22 can you comment on that ?

I think default '1' is meant to show that by default it is a single count of something, but you do not know what. Could be 1 second (timer), could be 1 transaction (counter), could be one byte, kb, mb, or gb. Without having a unit provided it is basically impossible to infer what the value should be, so it becomes "1 of something".

@dyladan mentioned it correctly.

packages/opentelemetry-metrics/src/BaseObserverMetric.ts

lykkin · 2020-06-22T18:50:58Z

packages/opentelemetry-metrics/src/BatchObserverMetric.ts

+    super(name, options, MetricKind.OBSERVER, resource);
+    this._maxTimeoutUpdateMS =
+      options.maxTimeoutUpdateMS ?? MAX_TIMEOUT_UPDATE_MS;
+    this._callback = callback || NOOP_CALLBACK;


Same comment about type guarding as above.

packages/opentelemetry-metrics/src/ValueRecorderMetric.ts

obecny · 2020-06-29T12:18:13Z

@open-telemetry/javascript-approvers @open-telemetry/javascript-maintainers ^^

dyladan

Big change, but I'm satisfied all the concerns are met at this point. Looks good and we can always make minor tweaks later if need be.

examples/metrics/metrics/observer.js

packages/opentelemetry-metrics/README.md

examples/metrics/metrics/observer.js

packages/opentelemetry-metrics/README.md

mayurkale22 · 2020-06-30T06:54:57Z

packages/opentelemetry-metrics/src/Metric.ts

-      description: this._options.description,
-      unit: this._options.unit,
+      description: this._options.description || '',
+      unit: this._options.unit || '1',


@dyladan mentioned it correctly.

mayurkale22 · 2020-06-30T07:07:01Z

packages/opentelemetry-metrics/src/Metric.ts

@@ -39,13 +31,16 @@ export abstract class Metric<T extends BaseBoundInstrument>

  constructor(
    private readonly _name: string,
-    private readonly _options: MetricOptions,
+    private readonly _options: api.MetricOptions,


IIRC we have intentionally used MetricOptions instead of api.MetricOptions in order to get ride of Non-Null Assertion Operator!. This way no need to worry about missing values in the SDK and no need to assign default values the way you did here: https://github.com/open-telemetry/opentelemetry-js/pull/1137/files#diff-c46db629f283d0e2d7d273379302ec6fR94. Does it make sense?

Related comment: #415 (comment)

hmm ok but then the api.MetricOptions was used anyway as an interface in our sdk so this is imho duplication. Also our sdk needs to be compatible with api so our sdk needs to use the api.MetricOptions anyway and you have to sync it manually. With regards to missing values maybe I'm missing something but if we expect our sdk to work with our api we should create a default values anyway not matter if we use a separate options interface or not as our sdk should work with anything (any 3rd party) that is api compatible and vice versa. I might revert it but I really think this is duplication and we endup with casting the interface once as api and once as our sdk and still we will have problem I mentioned.

I largely agree with your comment. This kind of duplication is adding more confusion than initially intended. Also, it is hard to keep them in sync for longer runs.

packages/opentelemetry-metrics/src/types.ts

packages/opentelemetry-metrics/src/BatchObserverMetric.ts

mayurkale22 · 2020-06-30T18:07:23Z

Sorry for the delay in reviews. It would be great if you can split PRs into smaller ones to get faster approval and merge. Renaming aggregators and refactoring other metrics would have easily made into separate PR. Although you have mentioned everything in the description, the PR title doesn't justify whole work.

obecny · 2020-06-30T19:23:04Z

Sorry for the delay in reviews. It would be great if you can split PRs into smaller ones to get faster approval and merge. Renaming aggregators and refactoring other metrics would have easily made into separate PR. Although you have mentioned everything in the description, the PR title doesn't justify whole work.

Sry for confusion, my initial thought was that it will be faster in one PR, but next time I would definitely split this into smaller chunks. Basically some of the things were discovered during coding and was already hard to go back - the temptation to fix things was too big if you know what I mean :D

dyladan · 2020-07-01T13:03:17Z

🎉

* chore: adding batch observer, some metrics refactoring * chore: undo changes after testing * chore: undo changes after testing * chore: addressing comments * chore: renaming observer into value observer, fixing few spotted issues * chore: missing renamed for ValueObserver * chore: removing unused class * chore: cleanup * chore: refactoring, renaming aggregators * chore: refactoring observer to have base class that can be extended * chore: changing aggregator for ValueObserver, exposing batcher so it can be used to override a default one * chore: addressing comments * chore: addressing comments * chore: preventing user from updating observer after timeout or update * chore: aligning aggregators for value observer and recorder with regards to last spec changes * chore: fixing test * chore: fixes after merge * chore: changes after review * chore: changes after review with some additional fixes around typing * chore: changes after review * chore: lint * chore: reviews * chore: typo

…1135 (open-telemetry#1137) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>

chore: adding batch observer, some metrics refactoring

5511aa9

obecny requested review from bg451, dyladan, markwolff, mayurkale22, mwear, OlivierAlbertini, rochdev and vmarchaud as code owners June 3, 2020 17:26

obecny self-assigned this Jun 3, 2020

obecny added size/XXL enhancement New feature or request labels Jun 3, 2020

obecny added 2 commits June 3, 2020 19:40

chore: undo changes after testing

72e00fb

chore: undo changes after testing

8e26b73

mayurkale22 reviewed Jun 3, 2020

View reviewed changes

examples/metrics/metrics/observer.js Outdated Show resolved Hide resolved

packages/opentelemetry-api/src/metrics/BatchObserverResult.ts Outdated Show resolved Hide resolved

packages/opentelemetry-metrics/src/Meter.ts Show resolved Hide resolved

dyladan reviewed Jun 3, 2020

View reviewed changes

obecny added 3 commits June 4, 2020 17:55

chore: addressing comments

2e8c892

Merge branch 'master' into batch-observer

8f1d3e6

chore: renaming observer into value observer, fixing few spotted issues

2d55754

obecny requested a review from naseemkullah as a code owner June 4, 2020 16:21

obecny added 2 commits June 4, 2020 18:37

chore: missing renamed for ValueObserver

54448d4

chore: removing unused class

e8cf27b

obecny added the breaking label Jun 4, 2020

obecny added 3 commits June 4, 2020 22:52

chore: cleanup

36fdc5b

chore: refactoring, renaming aggregators

353c476

chore: merge branch 'master' into batch-observer

4983115

chore: changes after review

1874414

obecny requested a review from dyladan June 12, 2020 22:30

markwolff reviewed Jun 17, 2020

View reviewed changes

chore: merge branch 'master' into batch-observer

18a8a86

lykkin reviewed Jun 22, 2020

View reviewed changes

obecny requested a review from legendecas as a code owner June 22, 2020 20:16

chore: lint

133625f

vmarchaud mentioned this pull request Jun 24, 2020

feat(prom): expose measure as summary #982 #1038

Closed

dyladan approved these changes Jun 29, 2020

View reviewed changes

mayurkale22 reviewed Jun 30, 2020

View reviewed changes

packages/opentelemetry-metrics/src/BatchObserverMetric.ts Outdated Show resolved Hide resolved

obecny added 3 commits June 30, 2020 14:20

chore: merge branch 'master' into batch-observer

5e52e03

chore: reviews

2214c9f

chore: typo

f1ac953

obecny requested a review from mayurkale22 June 30, 2020 13:31

markwolff approved these changes Jun 30, 2020

View reviewed changes

lykkin approved these changes Jun 30, 2020

View reviewed changes

mayurkale22 approved these changes Jun 30, 2020

View reviewed changes

mayurkale22 merged commit 9845b4f into open-telemetry:master Jun 30, 2020

obecny mentioned this pull request Jul 1, 2020

Remove metric kind "Observer" #1146

Closed

obecny deleted the batch-observer branch July 8, 2020 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch observer #1137

Batch observer #1137

obecny commented Jun 3, 2020 •

edited

Loading

dyladan commented Jun 3, 2020

codecov bot commented Jun 3, 2020 •

edited

Loading

obecny commented Jun 3, 2020

mayurkale22 left a comment

dyladan left a comment

obecny commented Jun 4, 2020

markwolff left a comment

markwolff Jun 17, 2020

obecny Jun 22, 2020

markwolff Jun 17, 2020

obecny Jun 22, 2020 •

edited

Loading

dyladan Jun 29, 2020

mayurkale22 Jun 30, 2020

lykkin Jun 22, 2020

obecny commented Jun 29, 2020

dyladan left a comment

mayurkale22 Jun 30, 2020

mayurkale22 Jun 30, 2020

obecny Jun 30, 2020

mayurkale22 Jun 30, 2020

mayurkale22 commented Jun 30, 2020

obecny commented Jun 30, 2020

dyladan commented Jul 1, 2020

Batch observer #1137

Batch observer #1137

Conversation

obecny commented Jun 3, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

dyladan commented Jun 3, 2020

codecov bot commented Jun 3, 2020 • edited Loading

Codecov Report

obecny commented Jun 3, 2020

mayurkale22 left a comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

obecny commented Jun 4, 2020

markwolff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny commented Jun 29, 2020

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jun 30, 2020

obecny commented Jun 30, 2020

dyladan commented Jul 1, 2020

obecny commented Jun 3, 2020 •

edited

Loading

codecov bot commented Jun 3, 2020 •

edited

Loading

obecny Jun 22, 2020 •

edited

Loading