-
Notifications
You must be signed in to change notification settings - Fork 824
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
Batch observer #1137
Batch observer #1137
Conversation
Does this also contain the changes we talked about for the non-batch observer? |
Codecov Report
@@ 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
|
Yes - it has both of them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is very big, easy to miss the things. Will take a look again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't have a lot of time to look at this today. It's on my list for first thing tomorrow.
I have splited the code from Meter into separate files as Meter was growing bigger and bigger (different classes in one file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but I'll take another closer look
resource: Resource, | ||
metricKind: MetricKind, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reorder these two to match Metric ctor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they already match the other metric like CounterMetric
, ValueRecorderMetric
etc.
description: this._options.description, | ||
unit: this._options.unit, | ||
description: this._options.description || '', | ||
unit: this._options.unit || '1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dyladan mentioned it correctly.
super(name, options, MetricKind.OBSERVER, resource); | ||
this._maxTimeoutUpdateMS = | ||
options.maxTimeoutUpdateMS ?? MAX_TIMEOUT_UPDATE_MS; | ||
this._callback = callback || NOOP_CALLBACK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about type guarding as above.
@open-telemetry/javascript-approvers @open-telemetry/javascript-maintainers ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
description: this._options.description, | ||
unit: this._options.unit, | ||
description: this._options.description || '', | ||
unit: this._options.unit || '1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dyladan mentioned it correctly.
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
🎉 |
* 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
* 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>
…1135 (open-telemetry#1137) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
…1135 (open-telemetry#1137) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Which problem is this PR solving?
Short description of the changes