-
Notifications
You must be signed in to change notification settings - Fork 773
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
Make Delta vs Cumulative based purely on Exporter #2142
Conversation
Codecov Report
@@ Coverage Diff @@
## metrics #2142 +/- ##
===========================================
- Coverage 78.27% 78.24% -0.03%
===========================================
Files 212 212
Lines 6632 6634 +2
===========================================
Hits 5191 5191
- Misses 1441 1443 +2
|
/// Gets or sets a value indicating whether to export Delta | ||
/// values or not (Cumulative). | ||
/// </summary> | ||
public bool IsDelta { get; set; } = true; |
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 a boolean is fine, but just pondering another idea... what if we had an enumeration for AggregationTemporality
and this setting was instead
public AggregationTemporality AggregationTemporality { get; set; } = AggregationTemporality.Delta
Also, this brings up an interesting question of defaults. What's the sense from the spec regarding defaults for SDK provided exporters?
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.
+1 with AggregationTemporality
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 defaults to delta makes more sense (for console, delta generates less output). A good topic for the SDK spec :)
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.
Agree. making it in next PR :)
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.
LGTM
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.
LGTM.
Changes
Makes Delta vs Cumulative decision tied to exporter. Its an exporter level setting, so all metrics either becomes delta or cumulative.
Once Views are in, this'd require rework anyway, as views likely would allow per-instrument delta vs cumulative setting.
Modified ConsoleExporter to pick between delta and cumulative, defaulting to delta
Cumulative Console Exporter
Delta Console Exporter