Skip to content
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

[receiver/dockerstats] Incorrect metric naming? #21807

Closed
carlossscastro opened this issue May 11, 2023 · 4 comments
Closed

[receiver/dockerstats] Incorrect metric naming? #21807

carlossscastro opened this issue May 11, 2023 · 4 comments
Labels
bug Something isn't working receiver/dockerstats

Comments

@carlossscastro
Copy link
Contributor

Component(s)

receiver/dockerstats

What happened?

Description

While working on #21803 I noticed that the metric reporting the cpu utilization percentage of a container is named container.cpu.percent
As per OTel semantic rules this would be better named as container.cpu.utilization.

Other references:
https://opentelemetry.io/docs/specification/otel/metrics/semantic_conventions/process-metrics/
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/hostmetricsreceiver/internal/scraper/cpuscraper/documentation.md#systemcpuutilization

Expected Result

container.cpu.utilization

Actual Result

container.cpu.percent

Collector version

any

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

I'm aware this might be a breaking change therefore would like to take the opportunity to fix it now while in alpha.
If agreed by the maintainers, I'll be happy to submit a quick PR to fix this.

Thanks!

@carlossscastro carlossscastro added bug Something isn't working needs triage New item requiring triage labels May 11, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@andrzej-stencel
Copy link
Member

The semantic rules link is already invalid - things change fast 😅 - I think this is the up-to-date link: https://opentelemetry.io/docs/specs/otel/metrics/semantic_conventions/#instrument-naming.

I agree that container.cpu.percent should be named container.cpu.utilization.

As you've mentioned, this component is in "alpha" stability level. I wonder if we should still apply the breaking change rules, specifically by adding a feature gate for this change. I have a feeling we should.

@andrzej-stencel andrzej-stencel removed the needs triage New item requiring triage label May 19, 2023
@mx-psi mx-psi changed the title Incorrect metric naming? [receiver/dockerstats] Incorrect metric naming? May 19, 2023
@carlossscastro
Copy link
Contributor Author

carlossscastro commented May 22, 2023

@astencel-sumo

Thanks for the suggestion and guidance.
I have submitted a PR for this change #22823

@carlossscastro
Copy link
Contributor Author

Closing this issue as PR has been merged

dmitryax pushed a commit that referenced this issue Oct 13, 2023
2nd step for the deprecation of `container.cpu.percent`

According to the deprecation plan in the
[docs](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/v0.79.0/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification),
this PR disables the old metric by default, to be released in v0.83.0

tracking issue:
#21807

---------

Co-authored-by: Christian <[email protected]>
JaredTan95 pushed a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this issue Oct 18, 2023
…24183)

2nd step for the deprecation of `container.cpu.percent`

According to the deprecation plan in the
[docs](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/v0.79.0/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification),
this PR disables the old metric by default, to be released in v0.83.0

tracking issue:
open-telemetry#21807

---------

Co-authored-by: Christian <[email protected]>
dmitryax pushed a commit that referenced this issue Oct 26, 2023
…mplete the transition to container.cpu.utilization (#27795)

**Description:** <Describe what has changed.>

Following up #24183, this PR removes the deprecated
`container.cpu.percent` metric as explained in README's [deprecation
section](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification).

**Link to tracking Issue:** #21807

**Testing:** <Describe what testing was performed and which tests were
added.>

```
❯ go test -race -timeout 300s -parallel 4 --tags="" ./... -count 1
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver  1.480s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata        1.525s
```

**Documentation:** the documentation has been updated using `mdatagen`

---------

Co-authored-by: Pablo Baeyens <[email protected]>
sigilioso added a commit to carlossscastro/opentelemetry-collector-contrib that referenced this issue Oct 27, 2023
…mplete the transition to container.cpu.utilization (open-telemetry#27795)

**Description:** <Describe what has changed.>

Following up open-telemetry#24183, this PR removes the deprecated
`container.cpu.percent` metric as explained in README's [deprecation
section](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification).

**Link to tracking Issue:** open-telemetry#21807

**Testing:** <Describe what testing was performed and which tests were
added.>

```
❯ go test -race -timeout 300s -parallel 4 --tags="" ./... -count 1
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver  1.480s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata        1.525s
```

**Documentation:** the documentation has been updated using `mdatagen`

---------

Co-authored-by: Pablo Baeyens <[email protected]>
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
…24183)

2nd step for the deprecation of `container.cpu.percent`

According to the deprecation plan in the
[docs](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/v0.79.0/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification),
this PR disables the old metric by default, to be released in v0.83.0

tracking issue:
open-telemetry#21807

---------

Co-authored-by: Christian <[email protected]>
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
…mplete the transition to container.cpu.utilization (open-telemetry#27795)

**Description:** <Describe what has changed.>

Following up open-telemetry#24183, this PR removes the deprecated
`container.cpu.percent` metric as explained in README's [deprecation
section](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification).

**Link to tracking Issue:** open-telemetry#21807

**Testing:** <Describe what testing was performed and which tests were
added.>

```
❯ go test -race -timeout 300s -parallel 4 --tags="" ./... -count 1
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver  1.480s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata        1.525s
```

**Documentation:** the documentation has been updated using `mdatagen`

---------

Co-authored-by: Pablo Baeyens <[email protected]>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue Nov 24, 2023
…mplete the transition to container.cpu.utilization (open-telemetry#27795)

**Description:** <Describe what has changed.>

Following up open-telemetry#24183, this PR removes the deprecated
`container.cpu.percent` metric as explained in README's [deprecation
section](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/dockerstatsreceiver#transition-to-cpu-utilization-metric-name-aligned-with-opentelemetry-specification).

**Link to tracking Issue:** open-telemetry#21807

**Testing:** <Describe what testing was performed and which tests were
added.>

```
❯ go test -race -timeout 300s -parallel 4 --tags="" ./... -count 1
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver  1.480s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata        1.525s
```

**Documentation:** the documentation has been updated using `mdatagen`

---------

Co-authored-by: Pablo Baeyens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working receiver/dockerstats
Projects
None yet
Development

No branches or pull requests

2 participants