Mark exemplars as stable. #3870
Merged
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
jack-berg
reviewed
Feb 8, 2024
8 tasks
|
Are we in need of the usual 3 languages implemented before calling this stable? The spec matrix shows only Java, though .NET has most of the implementation (in preview). I can send an update to spec matrix to reflect .NET's status. |
|
Those link does not tell much...For example, in JavaScript some exemplar related classes are there, but its not connected to SDK - its as good as saying JS does not support Exemplars... Which is why I was hoping every languages who implemented it will update the matrix, so we know if we have 3 languages implementing the spec. (or we can ask around as well :) ) OTel C++ also has some exemplars.. |
|
Go has an experimental implementation. It does not expose any API for configuration outside of environment variables. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
The TC discussed this during the Mar. 13th, 2024 meeting:
|
jack-berg
added a commit
that referenced
this pull request
Mar 20, 2024
…#3943) Related to #3756. See this [conversation](#3870 (comment)) for context. --------- Co-authored-by: Reiley Yang <reyang@microsoft.com>
|
This is now ready for review/merge |
jack-berg
reviewed
Mar 21, 2024
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
#3994 would hopefully be the last piece before we mark Exemplar spec stable. Please refer to the #3944 PR description for the Technical Committee discussion details. @cijothomas would you check if it unblocks this PR? Thanks! |
Sure. Will check and post back here. |
cijothomas
approved these changes
Apr 13, 2024
There was a problem hiding this comment.
Based on #3994 (comment), languages are free to chose a different default, and changing the default is considered non-breaking.
All the blocking issues are hence, addressable post stable as well.
reyang
approved these changes
Apr 16, 2024
|
@open-telemetry/specs-approvers @open-telemetry/specs-metrics-approvers @open-telemetry/technical-committee PTAL, I think this is ready to be merged by end of Fri. this week (Apr. 19th, 2024, PT). |
jack-berg
approved these changes
Apr 17, 2024
|
open-telemetry/opentelemetry-dotnet#5545 (comment) OTel .NET's performance hit when enabling exemplars is shown here. And OTel .NET is planning to make Exemplars opt-in for now. @jack-berg Do you have some perf numbers from Java handy that can be shared? |
jmacd
approved these changes
Apr 19, 2024
|
I measured the Lightstep metrics SDK implementation of exemplars here and the performance hit is consistent with what has been reported for .NET. For example, a fast-path that was 45ns becomes 60ns, for a 33% hit. I believe this is acceptable, and if there are applications for which a few 10s or 100s of nanoseconds of additional cost per metrics event are limiting, there may be other paths to optimization other than simply disabling exemplars. |
|
Here are the benchmarks from the java repo. Sharing these without having recently dug into what elements of the exemplar implementation drive perf. The benchmark tests a variety of dimension:
There's a lot of noise here, so I've narrowed it down to show the time difference with a single thread and exemplars enabled and disabled: There's no doubt a performance impact to enabling examplars, but to add some context:
|
|
Benchmark after optimizing accessing the current nano time from the VM below. Much more in line with what we're seeing from .NET and Go. |
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.
Fixes #3756
Changes
For non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changesspec-compliance-matrix.mdupdated if necessary