-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove metric processor Subtract function; rename ExportKind to AggregationTemporality #1415
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1415 +/- ##
=======================================
- Coverage 79.0% 78.9% -0.1%
=======================================
Files 127 127
Lines 6683 6666 -17
=======================================
- Hits 5280 5262 -18
Misses 1148 1148
- Partials 255 256 +1
|
@open-telemetry/specs-metrics-approvers As I am trying to bring the OTel-Go metrics SDK into line with "Everything we will specify", this is something I'd like to remove. (Note: I'm hoping to relieve the @open-telemetry/go-approvers of this responsibility, it's just code deletion.) |
What was an example use-case for this functionality? Something like an absolute system counter (# of network packets sent) that the backend system wants exposed as a delta? I presume this does not affect any synchronous instruments, which will still support being exported as a delta even if they are an absolute count? |
Yes @evantorrie something like that. I updated the PR description to clarify some use-cases, but I think the term confusion has a lot to do with "ExportKind", which I've renamed as we have already discussed in an open spec PR (open-telemetry/opentelemetry-specification#1159 (comment)).
@krnowak Yes, but I don't really think there are any of these out there. An exporter could use |
@bogdandrutu Please review. |
PTAL |
@bogdandrutu please take a look. |
Closing as the metrics signal refactor at the specification level might make this work wasted. We can open again if the specification solidifies and it is determined needed. |
The basic Processor has code to convert cumulative values to delta values on the export path. This was exploratory functionality, and has been requested a few times, but is not considered widely useful. This removes it completely.
There are not many known use-cases for this support for it to be included in the basic processor, but for example:
process.cpu.time
as a DELTA not a CUMULATIVE.UpDownSumObserver
instrument, with say 2 dimensions input by the observer callback and 1 dimension output (usingHistogram
type); then configures exemplar output (usingRawValue
type) with the complete two dimensions; exemplar sampling should be performed using DELTA weights to select the items most responsible for the rise/fall of the sum.As you can see these are a bit contrived, and I think the exporters that want these things can easily take a DIY approach.
This PR also renames
ExportKind
toAggregationTemporality
, as this terminology was already flagged as confusing in an open spec PR: open-telemetry/opentelemetry-specification#1159 (comment)