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.

Sign up for GitHub

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

Jump to bottom

Docker API version float64 #24025

Closed
b0kky opened this issue Jul 7, 2023 · 10 comments
Closed

Docker API version float64 #24025

b0kky opened this issue Jul 7, 2023 · 10 comments
Labels
bug Something isn't working receiver/dockerstats

Comments

@b0kky
Copy link

b0kky commented Jul 7, 2023

Component(s)

receiver/dockerstats

What happened?

Description

Docker API with round versions (1.20, 1.30, 1.40, etc) converted to 1.2, 1.3, 1.4 etc

Steps to Reproduce

Enable dockerstatreceiver and set round api_version , for example 1.40:

    docker_stats:
        api_version: 1.40

Expected Result

This config will set api_version as expected: 1.40

Actual Result

opentelemetry restarting with error:

Error: cannot start pipelines: Error response from daemon: client version 1.4 is too old. Minimum supported API version is >1.12, please upgrade your client to a newer version

At docker documentation described version, and you can see "round" version : 1.20, 1.30 etc.
https://docs.docker.com/engine/api/#api-version-matrix

Collector version

0.80.0

Environment information

Environment

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

OpenTelemetry Collector configuration

data:
      receivers:
        docker_stats:
          endpoint: unix:///var/run/docker.sock
          collection_interval: 10s
          timeout: 20s
          api_version: 1.40
-----------
          metrics:
            receivers:
              - docker_stats
            processors:
              - batch
              - metricstransform
            exporters:
              - prometheus

Log output

Error: cannot start pipelines: Error response from daemon: client version 1.4 is too old. Minimum supported API version is 1.12, please upgrade your client to a newer version

Additional context

In a source receiver/dockerstatsreceiver/config.go , set

`DockerAPIVersion float64 `mapstructure:"api_version"

While uses float64, 1.40 will convert to 1.4

$ python -c 'print(float(1.40))'
1.4
@b0kky b0kky added bug Something isn't working needs triage New item requiring triage labels Jul 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Pinging code owners:

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

@b0kky
Copy link
Author

b0kky commented Jul 11, 2023

@rmfitzpatrick @jamesmoessis any news?

@rmfitzpatrick
Copy link
Contributor

This is indeed a bug, but to support the float config value type I think we need a custom api version type. I've tried working on this briefly but mapstructure appears to only support encoding.TextUnmarshaler implementations for strings? Will keep looking and unfortunately using 1.39 or 1.41 may be the only "workaround" at this time as is since the DOCKER_API_VERSION env var isn't respected by the factory.

@b0kky
Copy link
Author

b0kky commented Jul 11, 2023

@rmfitzpatrick appreciate for your answer, and work. Yeap, using not round versions is workaround for this bug

This was referenced Sep 3, 2023
Open
Open
@crobert-1
Copy link
Member

/label -needs-triage

@github-actions github-actions bot removed the needs triage New item requiring triage label Sep 5, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

@github-actions github-actions bot added the Stale label Nov 6, 2023
@crobert-1 crobert-1 removed the Stale label Nov 7, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 8, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

@github-actions github-actions bot added the Stale label Jan 8, 2024
@crobert-1 crobert-1 removed the Stale label Jan 8, 2024
@rmfitzpatrick rmfitzpatrick mentioned this issue Jan 12, 2024
Merged
dmitryax pushed a commit that referenced this issue Jan 17, 2024
@rmfitzpatrick
docker: adopt api_version as string to correct float limitations (#30480
f2dc31e 
)

**Description:**
Fixing a bug - currently the docker client api version is a float, which
is incompatible w/ the actual engine api version scheme (1.40 != 1.4).
These changes adopt them as strings with validating helpers with minimum
version checking using the docker types api*.

Because [mapstructure's text unmarshaller
hook](https://github.com/mitchellh/mapstructure/blob/63cde0dfe2481856bcfc2184477b26df770f19d7/decode_hooks.go#L266)
only applies to strings I wasn't able to find a way to a complete fix
without the breaking change of requiring api versions being strings
(i.e. a user setting the value `1.40` will always be treated as a float
1.4)*. The proposed fix still suffers from the truncation problem but
now provides the `!!str` option to offer a workaround that didn't exist.

**Link to tracking Issue:**
#24025
mfyuce pushed a commit to mfyuce/opentelemetry-collector-contrib that referenced this issue Jan 18, 2024
@rmfitzpatrick @mfyuce
docker: adopt api_version as string to correct float limitations (ope…
90944a7 
…n-telemetry#30480)

**Description:**
Fixing a bug - currently the docker client api version is a float, which
is incompatible w/ the actual engine api version scheme (1.40 != 1.4).
These changes adopt them as strings with validating helpers with minimum
version checking using the docker types api*.

Because [mapstructure's text unmarshaller
hook](https://github.com/mitchellh/mapstructure/blob/63cde0dfe2481856bcfc2184477b26df770f19d7/decode_hooks.go#L266)
only applies to strings I wasn't able to find a way to a complete fix
without the breaking change of requiring api versions being strings
(i.e. a user setting the value `1.40` will always be treated as a float
1.4)*. The proposed fix still suffers from the truncation problem but
now provides the `!!str` option to offer a workaround that didn't exist.

**Link to tracking Issue:**
open-telemetry#24025
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Feb 1, 2024
@rmfitzpatrick @cparkins
docker: adopt api_version as string to correct float limitations (ope…
1db7428 
…n-telemetry#30480)

**Description:**
Fixing a bug - currently the docker client api version is a float, which
is incompatible w/ the actual engine api version scheme (1.40 != 1.4).
These changes adopt them as strings with validating helpers with minimum
version checking using the docker types api*.

Because [mapstructure's text unmarshaller
hook](https://github.com/mitchellh/mapstructure/blob/63cde0dfe2481856bcfc2184477b26df770f19d7/decode_hooks.go#L266)
only applies to strings I wasn't able to find a way to a complete fix
without the breaking change of requiring api versions being strings
(i.e. a user setting the value `1.40` will always be treated as a float
1.4)*. The proposed fix still suffers from the truncation problem but
now provides the `!!str` option to offer a workaround that didn't exist.

**Link to tracking Issue:**
open-telemetry#24025
@jamesmoessis
Copy link
Contributor

This can be closed right? #30480 fixes it. cc @MovieStoreGuy can you close it.

@jamesmoessis
Copy link
Contributor

Thanks @rmfitzpatrick for fixing

@b0kky
Copy link
Author

b0kky commented Feb 29, 2024

appreciate @rmfitzpatrick !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working receiver/dockerstats
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@rmfitzpatrick @MovieStoreGuy @jamesmoessis @crobert-1 @b0kky