feat(metrics): collect the DOCKER_HOST environment variable path#8007
feat(metrics): collect the DOCKER_HOST environment variable path#8007vicheey merged 25 commits intoaws:developfrom
Conversation
|
Thanks for the contribution! There are a few errors on At least in The first one is because of The other ones might be just errors on |
valerena
left a comment
There was a problem hiding this comment.
There are some more tests failing on test_installed_metrics.py
I added another comment to confirm the logic.
You can run pytest -vv tests/integration/telemetry to check those. They don't run with the standard make pr because they're integration tests, but they should run quickly (as opposed to other integration tests which could take hours)
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
3052dc8 to
ad611c8
Compare
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
|
@valerena, is the failing Windows smoke test related to this change? If so, I'm not sure how and could use some guidance on how to fix it |
|
There are some failing tests (related to logs cli command), although they don't seem to be related to the changes on this PR. |
|
This PR introduce new metric dimension to data. We need to update the data schema first before merging this PR. |
- Rename metric name from dockerHost to containerHost - Move containerHost from top-level metric to metricSpecificAttributes - Rename _get_docker_host() to _get_container_host() for tool-agnostic naming - Add containerHost collection only for command execution metrics - Update all tests to expect containerHost in correct location
6d954cc to
ce1e4ea
Compare
|
Ran |
|
Thanks for the fixes, @vicheey. LGTM |
| - unix://~/.rd/docker.sock -> rancher-desktop.sock -> rancher-desktop | ||
| - unix://~/.orbstack/run/docker.sock -> orbstack.sock -> orbstack |
There was a problem hiding this comment.
nit: extra space before rancher-desktop-.sock and orbstack.sock
Which issue(s) does this change fix?
Why is this change necessary?
It allows SAM CLI to have a proxy measurement on which tools users are using as its backend. This allows the SAM CLI team to prioritize testing and features from various tools which users may be using to provide their
DOCKER_HOST.How does it address the issue?
This change collects the last part of URIs/paths, in hope that these last parts shed light on which tool the user is using with SAM CLI. For example, if a user is using a local version of docker, this new metric would collect
docker.sock. This is only checking the last part of paths in an effort to not needlessly collect what may be private URLs to remote docker hosts which user's might be using.What side effects does this change have?
None?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.