-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support cumulative, delta, and pass-through exporters #799
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(I've figured out a better approach for the Aggregator API.) |
This was referenced Jun 11, 2020
This was referenced Jun 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm marking this a draft because it's a very large change. Several substantial portions of this change can be applied as separate PRs because they are independent. Otherwise, this PR is finished and unfortunately still a large change. More, inter-dependent parts of this PR may be broken apart afterward as long as the approach is agreed to.
The most significant changes are in
sdk/export/metric
, where:aggregation
subpackage is renamed fromaggregator
, newaggregation.Kind
is exposed.aggregation.Aggregation
interface is created, will be used by exporters; oneAggregator
contains twoAggregation
values, the "current" and "checkpoint" state.Aggregator
interface has been widened with new support, including access to current and checkpointAggregation
values, aSwap()
primitive, and an optionalSubtract()
primitive. The semantics ofMerge()
have changed to update the current state, not the checkpoint.Record
is now a type used between the Integrator and Exporter. Record includes the Aggregation appropriate for the exporter and the start and end time of the interval it represents (plus the descriptor, labels, and resource).Accumulation
is a new type like the formerRecord
: it's used between the Accumulator and Integrator, refers to an Aggregator and does not contain timestamps.Exporter
knows its own kind.Integrator
knows the set of (multiple) exporter kinds (in case of multiple exporters)CheckpointSet.ForEach
takes the exporter kind as an argument.I'm concerned about the complexity of the Aggregator API and am option to alternate suggestions. The change in Merge semantics--that it adds into the current state--combined with Swap(), makes it possible to combine values in place w/o extra memory allocations. In the ordinary case, the Accumulator owns one Aggregator per instrument and label set, and the Integrator owns a copy only when it needs to. The Accumulator makes use of the Aggregator Update/Checkpoint synchronization guarantee, while the Integrator doesn't require any locking and uses only the Merge/Swap/Subtract/CheckpointedValue/AccumulatedValue APIs.
With these fundamental changes, the
sdk/metric/integrator/simple
package is upgraded to handle cumulative instruments and cumulative exporters. It will use state when the instrument and exporter require it, either cumulative instrument with a delta exporter or a delta instrument with a cumulative exporter. All of the substantial changes in this PR happen in this package.Related TODO: SumObserver monotonicity is not being tested, but it could be done in the Integrator or the Accumulator.
Comment: The "simple" Integrator is no longer simple. Perhaps it should be renamed "basic". I expect that a configurable Integrator can be based off of this code, adding configuration support without having to re-implement the logic here.
Each of the current aggregators had to undergo changes to complete the change, changes which are large but not really functional changes. Only the "Sum" aggregator has been upgraded with support for
Subtract()
, as needed to expose deltas of cumulative instruments.Misc cleanups in this PR: Aggregator.Checkpoint() and Integrator.Process() no longer accept Context arguments, they weren't being used and create substantial confusion.