Add support to finish instrument recording#7792
Add support to finish instrument recording#7792atoulme wants to merge 7 commits intoopen-telemetry:mainfrom
Conversation
atoulme
force-pushed
the
remove_support
branch
from
02fedd7 to
d3f4d8c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7792 +/- ##
============================================
- Coverage 90.29% 90.26% -0.03%
- Complexity 7652 7668 +16
============================================
Files 843 852 +9
Lines 23066 23121 +55
Branches 2310 2313 +3
============================================
+ Hits 20827 20871 +44
- Misses 1520 1530 +10
- Partials 719 720 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
atoulme
changed the title
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Just as the spec was blocked on resolving the series start time tracking issue, we'll want this to incorporate the java implementation of series start time tracking (#8180).
With those changes and with the more detailed mechanics about what calling finish actually means for implementations, this is likely to get a bit tricky from an implementation standpoint.
I'm happy to collaborate with you on this if you don't mind me pushing commits to your branch (or alternatively I can create my own branch based off yours).
| } | ||
| AggregatorHolder<T> holderForRecord = getHolderForRecord(); | ||
| try { | ||
| holderForRecord.aggregatorHandles.remove(attributes); |
There was a problem hiding this comment.
As @dashpole points out, this is too naive as it removes series without a final report of their data.
| * @param context The explicit context to associate with this measurement. | ||
| * @since 1.56.0 | ||
| */ | ||
| default void finish(Attributes attributes, Context context) {} |
There was a problem hiding this comment.
Since there is no SDK functionality that depends on context, we don't need it as an argument.
| * Finish the instrument record. | ||
| * | ||
| * @param attributes A set of attributes to identify the instrument. | ||
| * @since 1.56.0 |
There was a problem hiding this comment.
#nit: we manaully add since annotations as part of the release process. This ensures that they accurately reflect the version changes were first published in. So just omit the annotation here.
| * @param attributes A set of attributes to identify the instrument. | ||
| * @since 1.56.0 | ||
| */ | ||
| default void finish(Attributes attributes) { |
There was a problem hiding this comment.
This really needs to be Predicate<Attributes> to be useful. As @dashpole points out, with this API, you effectively have to independently track the attributes you may want to later remove, which significantly reduces the usefulness.
2 participants
This is a POC implementation to support open-telemetry/opentelemetry-specification#2232