-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[processor/transform] unit is copied to count metric when using extract_count_metric()
#31575
Labels
Comments
pichlermarc
added
bug
Something isn't working
needs triage
New item requiring triage
labels
Mar 5, 2024
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
I'd be happy to open a PR if code-owners agree that we should change it. 🙂 |
Thanks for the detailed report! This is a good catch, I agree that a |
evan-bradley
added
priority:p2
Medium
and removed
needs triage
New item requiring triage
labels
Mar 5, 2024
TylerHelmuth
added a commit
that referenced
this issue
Mar 12, 2024
) **Description:** The `transformprocessor` offers a function `extract_count_metric()`. Currently this function copies over the `unit` from the original metric to a new `count` metric. However, this unit is not applicable as the value is a count of how many values were recorded in a Explicit Bucket/Exponential Histogram or Summary. Therefore this PR changes that function so that it adds the default unit (`1`) to the extracted count metric instead. **Link to tracking Issue:** #31575 **Testing:** Unit tests (updated) **Documentation:** Added a changelog entry as the change is user-facing (the unit of the emitted telemetry is changed) --------- Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
DougManton
pushed a commit
to DougManton/opentelemetry-collector-contrib
that referenced
this issue
Mar 13, 2024
…n-telemetry#31636) **Description:** The `transformprocessor` offers a function `extract_count_metric()`. Currently this function copies over the `unit` from the original metric to a new `count` metric. However, this unit is not applicable as the value is a count of how many values were recorded in a Explicit Bucket/Exponential Histogram or Summary. Therefore this PR changes that function so that it adds the default unit (`1`) to the extracted count metric instead. **Link to tracking Issue:** open-telemetry#31575 **Testing:** Unit tests (updated) **Documentation:** Added a changelog entry as the change is user-facing (the unit of the emitted telemetry is changed) --------- Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
XinRanZhAWS
pushed a commit
to XinRanZhAWS/opentelemetry-collector-contrib
that referenced
this issue
Mar 13, 2024
…n-telemetry#31636) **Description:** The `transformprocessor` offers a function `extract_count_metric()`. Currently this function copies over the `unit` from the original metric to a new `count` metric. However, this unit is not applicable as the value is a count of how many values were recorded in a Explicit Bucket/Exponential Histogram or Summary. Therefore this PR changes that function so that it adds the default unit (`1`) to the extracted count metric instead. **Link to tracking Issue:** open-telemetry#31575 **Testing:** Unit tests (updated) **Documentation:** Added a changelog entry as the change is user-facing (the unit of the emitted telemetry is changed) --------- Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Component(s)
processor/transform
What happened?
Description
Using
extract_count_metric()
will retain theunit
of the original metric. While this makes sense for extractedsum
metrics, it may not be accurate in most (all?) cases of extractedcount
metrics.Example (
http.server.request.duration
Histogram):Its
unit
iss
(seconds). In an app, we record how many seconds a request takes. Thesum
will be in seconds, as we just add those seconds together, so we should keep the unit there. However, forcount
theunit
cannot be seconds as it's actually the number of how many requests were recorded. I think (if I'm not missing anything) that the unit for extractedcount
should always be1
.Steps to Reproduce
Prerequisite files
collector-config.yaml
request.json
Steps:
otelcol-contrib --config collector-config.yaml
curl -X POST -H "Content-Type: application/json" -d @request.json -i localhost:4318/v1/metrics
unit
as the source metric)Expected Result
s
on the extracted metricActual Result
s
the extracted metricCollector version
v0.95.0
Environment information
Environment
OS: Ubuntu 22.04
OpenTelemetry Collector configuration
Log output
Additional context
opentelemetry-collector-contrib/processor/transformprocessor/internal/metrics/func_extract_count_metric.go
Line 47 in 0bd3c27
README.md
(perhaps it was not intended to be copied?)opentelemetry-collector-contrib/processor/transformprocessor/README.md
Line 236 in 0bd3c27
The text was updated successfully, but these errors were encountered: