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] Add inspect metrics #21803

Merged
merged 149 commits into from
Nov 18, 2023

Conversation

carlossscastro
Copy link
Contributor

@carlossscastro carlossscastro commented May 11, 2023

Description:

Adds 3 new metrics from the docker inspect api

  • container.cpu.limit
  • container.cpu.shares
  • container.restart.count

Since all these metrics are collected from the inspect api, a new method was created (recordInspectMetrics) where only the containerJSON structure is passed as an argument.

Link to tracking Issue: #21087

Testing:

New metrics were added to the receiver test and expected_metrics.yaml files.
The receiver and config tests were also adjusted to reflect the minimum api version of 1.25

❯ go clean -testcache
❯ make test
go test -race -timeout 300s -parallel 4 --tags="" ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver	0.508s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata	0.381s
❯ go test --tags integration ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver	14.060s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata	0.241s

Documentation:
New metrics documentation was generated using mdatagen

@carlossscastro carlossscastro requested review from a team and andrzej-stencel May 11, 2023 13:47
@carlossscastro carlossscastro changed the title Add inspect metrics [receiver/dockerstats] Add inspect metrics May 11, 2023
@andrzej-stencel
Copy link
Member

Shouldn't container.restart.count rather be named container.restarts?

According to the spec, count should only be used on namespaces. Is container.restarts a namespace that can include other metrics? If not, we should use container.restarts without the count suffix.

.chloggen/add_inspect_metrics.yaml Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/receiver.go Outdated Show resolved Hide resolved
Copy link
Member

@gbbr gbbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this Carlos! Left a small comment, hope it helps :)

receiver/dockerstatsreceiver/receiver.go Outdated Show resolved Hide resolved
@fatsheep9146
Copy link
Contributor

here is a failed check

Error: ../../receiver/dockerstatsreceiver/receiver.go:124:14: r.recordHostConfigMetrics undefined (type *metricsReceiver has no field or method recordHostConfigMetrics)
Error: ../../receiver/dockerstatsreceiver/receiver.go:287:10: use of package receiver not in selector

@sigilioso

@sigilioso
Copy link
Contributor

sigilioso commented Nov 15, 2023

Thanks for the heads up! It was an issue with one of the latest updates from main. It is fixed now: c374199

@gbbr
Copy link
Member

gbbr commented Nov 16, 2023

Why is this PR not getting merged? I see that everything is addressed and it has plenty of approvals.

@MovieStoreGuy MovieStoreGuy merged commit d7e1933 into open-telemetry:main Nov 18, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 18, 2023
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
**Description:**

Adds 3 new metrics from the docker inspect api
* container.cpu.limit
* container.cpu.shares
* container.restart.count

Since all these metrics are collected from the inspect api, a new method
was created (`recordInspectMetrics`) where only the containerJSON
structure is passed as an argument.

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

**Testing:**

New metrics were added to the receiver test and expected_metrics.yaml
files.
The receiver and config tests were also adjusted to reflect the minimum
api version of 1.25

```
❯ go clean -testcache
❯ make test
go test -race -timeout 300s -parallel 4 --tags="" ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver	0.508s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata	0.381s
❯ go test --tags integration ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver	14.060s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata	0.241s
```

**Documentation:**
New metrics documentation was generated using `mdatagen`

---------

Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Christian Kruse <[email protected]>
Signed-off-by: Dani Louca <[email protected]>
Signed-off-by: Jared Tan <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Raphael Silva <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Sean Porter <[email protected]>
Co-authored-by: Christian Felipe Álvarez <[email protected]>
Co-authored-by: Christian <[email protected]>
Co-authored-by: Ryan Fitzpatrick <[email protected]>
Co-authored-by: Martin Majlis <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
Co-authored-by: hovavza <[email protected]>
Co-authored-by: Povilas Versockas <[email protected]>
Co-authored-by: Dmitrii Anoshin <[email protected]>
Co-authored-by: sakulali <[email protected]>
Co-authored-by: OpenTelemetry Bot <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Co-authored-by: Paschalis Tsilias <[email protected]>
Co-authored-by: Juraci Paixão Kröhling <[email protected]>
Co-authored-by: Andrzej Stencel <[email protected]>
Co-authored-by: Brandon Johnson <[email protected]>
Co-authored-by: Miel Donkers <[email protected]>
Co-authored-by: bryan-aguilar <[email protected]>
Co-authored-by: Christian Kruse <[email protected]>
Co-authored-by: gord02 <[email protected]>
Co-authored-by: bagmeg <[email protected]>
Co-authored-by: Yang Song <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: VihasMakwana <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
Co-authored-by: Dani Louca <[email protected]>
Co-authored-by: xu0o0 <[email protected]>
Co-authored-by: David Ashpole <[email protected]>
Co-authored-by: Jared Tan <[email protected]>
Co-authored-by: Raj Nishtala <[email protected]>
Co-authored-by: Dominik Rosiek <[email protected]>
Co-authored-by: Priyanshu Raj <[email protected]>
Co-authored-by: Raphael Philipe Mendes da Silva <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Yotam loewenbach <[email protected]>
Co-authored-by: Alex Boten <[email protected]>
Co-authored-by: Jina Jain <[email protected]>
Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: Abhishek Saharn <[email protected]>
Co-authored-by: Ramachandran A G <[email protected]>
Co-authored-by: Ziqi Zhao <[email protected]>
Co-authored-by: Ramachandran A G <[email protected]>
Co-authored-by: Faith Chikwekwe <[email protected]>
Co-authored-by: Daniel Kuiper <[email protected]>
Co-authored-by: ArchangelSDY <[email protected]>
Co-authored-by: Pavol Loffay <[email protected]>
Co-authored-by: Paulo Janotti <[email protected]>
Co-authored-by: Nathan Burke <[email protected]>
Co-authored-by: shalper2 <[email protected]>
Co-authored-by: 道君 <[email protected]>
Co-authored-by: Colin Desmond <[email protected]>
Co-authored-by: Mario <[email protected]>
Co-authored-by: Paulin Todev <[email protected]>
Co-authored-by: Steven Swartz <[email protected]>
Co-authored-by: Chris Parkins <[email protected]>
Co-authored-by: Frederic Branczyk <[email protected]>
Co-authored-by: Jiekun <[email protected]>
Co-authored-by: Joel <[email protected]>
Co-authored-by: Sean Porter <[email protected]>
Co-authored-by: Tigran Najaryan <[email protected]>
Co-authored-by: nzbart <[email protected]>
Co-authored-by: Pavan Krishna <[email protected]>
Co-authored-by: pszkamruk-splunk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.