-
Notifications
You must be signed in to change notification settings - Fork 564
docs: Add metric doc #2946
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
docs: Add metric doc #2946
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2946 +/- ##
=====================================
Coverage 81.5% 81.5%
=====================================
Files 126 126
Lines 24796 24796
=====================================
+ Hits 20209 20214 +5
+ Misses 4587 4582 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
docs/metrics.md
Outdated
frequently. Instruments are fairly expensive and meant to be reused. For most | ||
applications, instruments can be created once and re-used. Instruments can also | ||
be cloned to create multiple handles to the same instrument, but the cloning | ||
should not be on hot path, but instead the cloned instance should be stored and |
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's not that bad to clone the instrument as it should only be a matter of incrementing the Arc atomic value, right?
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.
its not as bad as creating new one repeatedly, but the best option is to create/clone and re-use.
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 agree that the best thing to do is create/clone and re-use. In my opinion, I find exaggerating the dangers of a not-so-harmful operation a bit odd. We ideally want to mention only those things that we want the users to look out for and not overstate things to lessen the burden on the user.
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 cool! I think user-focussed documentation is great :)
Couple of minor comments inline.
last export is exported, allowing SDK to "forget" previous state. | ||
|
||
### Pre-Aggregation | ||
|
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 might go a bit to intended audience ( I think it is end users, right ? ) - but can we be more prescriptive here?
You should generally do X unless you need to do why, then do Z. Here's some more details on that in depth: [....]
So you don't have to read through and fully grok everything to get there
docs/metrics.md
Outdated
quickly lead to cardinality issues, resulting in metrics being capped. | ||
|
||
A better alternative is to use a concept in OpenTelemetry called | ||
[exemplars](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplar). |
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.
TIL!
|
||
#### Cardinality Limits - Implications | ||
|
||
Cardinality limits are enforced for each export interval, meaning the metrics |
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.
move this to within the temporality part, or remove this since this is covered
docs/metrics.md
Outdated
|
||
* If the dimension is static throughout the process lifetime (e.g. the name of | ||
the machine, data center): | ||
* If the dimension applies to all metrics, model it as Resource, or even |
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.
add something about - this attributes are unaffected by cardinality capping and so queried using just these would remain accurate.
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 could go into the opentelemetry crate that way it can be rendered with the crate and it could be referenced from any of the other crates easily as well.
I added a status - work in progress for this document. There are few suggestions that is pending, and there are many TODOs left still. Expecting to work on it iteratively. |
Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
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, well written. Further improvements can be in subsequent PRs.
This is put into a new docs location. I am open to suggestion on where is the best place to host this.
Fixes #2902 #1060