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] remove memory swap metrics #21890

Merged
merged 2 commits into from
May 23, 2023

Conversation

carlossscastro
Copy link
Contributor

@carlossscastro carlossscastro commented May 12, 2023

Description:
Metrics container.memory.total_swap and container.memory.swap were removed as they are not reported by Docker API.
These metrics are not part of Memory Stats on both cgroupsV1 and cgroupsV2.

Link to tracking Issue: #21190

Testing:
receiver test was updated to exclude those 2 metrics.
Expected metrics yaml files did not need any change as those metrics were not included.

❯ go clean -testcache
❯ make test
go test -race -timeout 300s -parallel 4 --tags="" ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver	0.424s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata	0.322s
❯ go test --tags integration ./...
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver	9.503s
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata	0.213s

Documentation:
Metrics documentation was updated using mdatagen

@@ -220,14 +220,6 @@ metrics:
value_type: int
aggregation: cumulative
monotonic: true
container.memory.swap:
Copy link
Member

@dmitryax dmitryax May 17, 2023

Choose a reason for hiding this comment

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

Ideally, such changes should be done via a deprecation process.

  1. Add a deprecation message in the metadata.yaml to generate the deprecation warnings:
    warnings:
      if_configured: This metric is deprecated and will be removed after v0.80.0
  1. Remove the metrics after v0.80.0 release

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this too, but if the metrics were never reported and since the component is alpha, it would be ok to do this in one step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're ok with both approaches but as mentioned these metrics were never reported in any of the API versions AFAIK.
This is more of a cleanup task to avoid confusion.
But again, we're ok either way.

Copy link
Member

Choose a reason for hiding this comment

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

These metrics were disabled by default. Someone could've added them to their configuration with:

receivers:
  docker_stats:
    metrics:
      container.memory.swap:
        enabled: true
      container.memory.total_swap:
        enabled: true

and the user might never even realize that these metrics are not actually reported. When they upgrade, their configuration will be broken. To fix this, they would need to remove those metrics entries from the configuration, which doesn't sound like a lot to ask for. But on the other hand, perhaps we should be super nice and issue a warning before removing these metrics? I'm on a fence with this 😄

Copy link
Member

Choose a reason for hiding this comment

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

For such a niche situation I think we can lean on the Alpha stability here an do a breaking change in 1 go. @carlossscastro can you share the collector startup error message that appears if you try to start the collector with these metrics enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TylerHelmuth

There is no error on the collector when they are enabled but they are never reported.
For memory metrics the receiver iterates through a map of metric names/values returned by the docker API and matches them with a map of names/recorders that we want to report. Since those 2 metric names are not returned by the api the recorder method for them was never called which avoided any errors:
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/dockerstatsreceiver/receiver.go#L160-L199

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

I'm also good to apply this change if code owners agree to make an exception

@@ -220,14 +220,6 @@ metrics:
value_type: int
aggregation: cumulative
monotonic: true
container.memory.swap:
enabled: false
description: "The amount of swap currently used by the processes in this cgroup."
Copy link
Member

Choose a reason for hiding this comment

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

This would become

description: "[DEPRECATED] The amount of swap currently used by the processes in this cgroup."

@jamesmoessis
Copy link
Contributor

I think it's fine to remove it in one step if it never actually worked. No need for a featuregate IMO

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.

5 participants