-
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
Add a getting started doc for metric #2132
Conversation
Codecov Report
@@ Coverage Diff @@
## metrics #2132 +/- ##
===========================================
- Coverage 83.49% 83.47% -0.03%
===========================================
Files 211 211
Lines 6703 6703
===========================================
- Hits 5597 5595 -2
- Misses 1106 1108 +2
|
{ | ||
counter.Add( | ||
10, | ||
new KeyValuePair<string, object>("tag1", "value1"), |
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.
Is there a simpler way to do it in C#? e.g. counter.Add(10, ("tag1", "value1), ("tag2", "value2"))
?
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 am not aware of any easier ways. These are the .NET API.
public void Add(T delta);
public void Add(T delta, KeyValuePair<string, object?> tag);
public void Add(T delta, KeyValuePair<string, object?> tag1, KeyValuePair<string, object?> tag2);
public void Add(T delta, KeyValuePair<string, object?> tag1, KeyValuePair<string, object?> tag2, KeyValuePair<string, object?> tag3);
public void Add(T delta, ReadOnlySpan<KeyValuePair<string, object?>> tags);
public void Add(T delta, params KeyValuePair<string, object?>[] tags);
}); | ||
writeMetricTask.Start(); | ||
|
||
Console.WriteLine("Press enter to exit."); |
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.
Instead of taking user input, consider something simpler (e.g. wait for 5 seconds before exit), and put a placeholder comment for Flush (since most users might expect such behavior?).
@@ -24,6 +24,11 @@ public class MetricConsoleExporter : MetricProcessor | |||
{ | |||
private string name; | |||
|
|||
public MetricConsoleExporter() |
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 is a bit confusing to have the exporter class derived from processor class.
Not introduced by this PR so no need to block on it.
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.
yea. This is something that would be changed.
(Also the ConsoleExporter is not coming from the Exporter.Console package.. Will be fixing all of them in subsequent PRs)
|
||
<!-- markdownlint-disable MD013 --> | ||
```text | ||
Export[] 14:54:56.162 14:54:57.155 TestMeter:counter [tag1=value1;tag2=value2] SumMetricAggregator Value: 15977610, Details: Delta=True,Mon=True,Count=1597761,Sum=15977610 |
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: probably not related to this PR, "Mon" could be misleading and we might want "Monotonic".
"Delta=True" - consider something like "Temporality=Delta" (and "Temporality=Cumulative").
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. to be addressed when exporter is properly written inside Console exporter package.
Merging. Will address further comments as follow ups. |
Fixes #.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes