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

cli: document initial otel support and usage #19776

Merged
merged 3 commits into from Apr 22, 2024

Conversation

dvdksn
Copy link
Contributor

@dvdksn dvdksn commented Apr 10, 2024

Description

Information about initial OTel support in the Docker CLI, for collecting telemetry data about Docker CLI command invocations

Reviews

  • Technical review
  • Editorial review
  • Product review

@github-actions github-actions bot added area/engine Issue affects Docker engine/daemon area/configuration Relates to configuring containers labels Apr 10, 2024
Copy link

netlify bot commented Apr 10, 2024

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 003c6fe
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/66216c2fbadacc0008abb4e3
😎 Deploy Preview https://deploy-preview-19776--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dvdksn dvdksn changed the title engine: document initial otel support and usage cli: document initial otel support and usage Apr 11, 2024
@dvdksn dvdksn added the area/cli Relates to the CLI client label Apr 11, 2024
Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
@github-actions github-actions bot added the hugo Updates related to hugo label Apr 11, 2024
@dvdksn dvdksn added this to the Engine v26.1.0 milestone Apr 11, 2024
@dvdksn dvdksn added the status/do-not-merge Pull requests that are awaiting some event or decision before they can be merged. label Apr 11, 2024
@dvdksn dvdksn marked this pull request as ready for review April 11, 2024 14:34
@github-actions github-actions bot added the area/release Relates to CI or deployment label Apr 11, 2024
@laurazard
Copy link
Member

@milas @jsternberg does this LGTY? (the idea is to have a basic explanation of what is there right now, together with a minimal+easy to follow example)

content/config/otel.md Outdated Show resolved Hide resolved
Comment on lines +62 to +71
command:
- "--config.file=/etc/prometheus/prom.yml"
ports:
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if it would work to mount the config over the default one that's provided; I noticed the image has some default options set, and this would mean that we're currently removing some of the other ones (do we know if that has an effect on what it does? At least it looks like this would mean changing the default storage location);

docker image inspect --format '{{json .Config.Cmd}}' prom/prometheus | jq .
[
  "--config.file=/etc/prometheus/prometheus.yml",
  "--storage.tsdb.path=/prometheus",
  "--web.console.libraries=/usr/share/prometheus/console_libraries",
  "--web.console.templates=/usr/share/prometheus/consoles"
]

Copy link
Member

Choose a reason for hiding this comment

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

The idea was more to provide a smallest working example than to provide an exhaustive guide on prometheus, so I removed anything that wasn't essential. Maybe we could mount the new one on top of that so we don't need to specify --config.file, but I don't think we should worry about removing any configurations as long as they don't break the setup.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was indeed looking if we could use more of the defaults (to simplify the compose-file even further). Definitely not critical, just was curious (and went looking; doesn't it have a default location to look for?)


## Available metrics

Docker CLI currently exports a single metric, `command.time`, which measures
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording seems strange to me. I want this to be described more like below (with time listed with the other attributes):

The Docker CLI currently collects telemetry on each command executed. Each record has the following attributes:

  • command.name: the name of the command
  • command.time: the execution duration of the command in milliseconds
  • etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. I think we do need to separate the metric (IIUC: a floating point counter measurement) and its attributes (the key-value pairs associated with the primary data point, the time measurements). I will try to improve the wording

Copy link
Member

@laurazard laurazard Apr 12, 2024

Choose a reason for hiding this comment

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

This wording seems strange to me

this doesn't match the technical concepts though @sam-thibault.

OTel has a concept of metrics, and metrics have both names and attributes. Our naming is a little weird (also it's copied over from what is used in Buildx, etc. iirc) but in fact the metric is called command.time and, as attributes, contains command.name and the other ones mentioned.

Not sure how to get this across better, but it is important to explain that one is the metric name, since that's what users would have to use to look for the metrics in prometheus/anywhere else.

content/config/otel.md Outdated Show resolved Hide resolved
content/config/otel.md Outdated Show resolved Hide resolved
content/config/otel.md Outdated Show resolved Hide resolved
content/config/otel.md Show resolved Hide resolved
content/config/otel.md Outdated Show resolved Hide resolved
Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

LGTM! I had some pedantic comments about the OpenTelemetry "brand" name but the content is solid & comprehensive (😭 that a minimal OTel+Prom Compose project is still pretty complex, but not much we can do there)

data/toc.yaml Outdated Show resolved Hide resolved
content/config/otel.md Outdated Show resolved Hide resolved
content/config/otel.md Outdated Show resolved Hide resolved
Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
@dvdksn dvdksn requested a review from a team April 22, 2024 15:17
@dvdksn dvdksn added status/review and removed status/do-not-merge Pull requests that are awaiting some event or decision before they can be merged. labels Apr 22, 2024
Copy link
Contributor

@craig-osterhout craig-osterhout left a comment

Choose a reason for hiding this comment

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

LGTM

@dvdksn dvdksn merged commit 2a1b3e0 into docker:main Apr 22, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Relates to the CLI client area/configuration Relates to configuring containers area/engine Issue affects Docker engine/daemon area/release Relates to CI or deployment hugo Updates related to hugo status/review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants