-
Notifications
You must be signed in to change notification settings - Fork 523
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
base: main
Are you sure you want to change the base?
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.4% 81.4%
=====================================
Files 126 126
Lines 24319 24319
=====================================
Hits 19816 19816
Misses 4503 4503 ☔ 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.
docs/metrics.md
Outdated
|
||
#### Cardinality Limit - Implications | ||
|
||
Cardinality limits are enforced during 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.
Remove this sentence from this section since this is only true for Delta?
folded into the overflow bucket due to cardinality capping. | ||
|
||
* **All Attributes Affected**: When overflow occurs, it's not just | ||
high-cardinality attributes that are affected. The entire attribute |
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 description is not so different from the above one. I think we can rearrange these sections for simpler understanding:
This unpredictability creates several important considerations when querying metrics in any backend system:
- Total Accuracy: ...
- Attributes-based querying
- Only partial information retained: (this would be the "How many red apples were sold?" example). Measurements with a superset of dimensions could be folded into overflow. We only retained information for Downtown based measurements here. Value returned by query suggests that we at least sold those many red apples. It could have been more.
- No information retained: This would be the "How many items were sold in Midtown?" example. All measurements related to Midtown were folded into overflow. Value returned by the query is zero and it doesn't help as we may or may not have sold items in Midtown.
Does that make sense?
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
|
||
* 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.
Most use-cases require you to create ONLY one instance of MeterProvider. You | ||
should NOT create multiple instances of MeterProvider unless you have some | ||
unusual requirement of having different export strategies within the same | ||
application. Using multiple instances of MeterProvider requires users to |
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.
Can we mention thread-isolated runtimes for which per-thread meter provider instances would be quite common?
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