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

Initial otel implementation #4940

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Initial otel implementation #4940

merged 3 commits into from
Mar 28, 2024

Conversation

krissetto
Copy link
Contributor

@krissetto krissetto commented Mar 13, 2024

- What's this?
OTEL bits in the CLI

- Goal

The very first goal is to provide enough metrics to cover the compose wrapper's current usage (so we can then remove it).
To keep continuity with the current impl in the compose wrapper, these should include at minimum:

  • command name;
  • command duration;
  • command status ("success"/"failure");
  • command exit code

Pls note that the organization of the code is not final and will surely be adjusted as the implementation matures

- What I did

  • Added some utils for otel related ops
  • Implemented an initial version of otel metrics using the utils ^

- How I did it

Taking inspiration and adapting bits from buildx and the compose wrapper

- How to verify it

Manually. Automated tests still need to be defined

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@krissetto krissetto changed the title Initial otel implementation 🚧 Initial otel implementation 🚧 Mar 13, 2024
cli/command/telemetry.go Outdated Show resolved Hide resolved
cli/command/telemetry.go Outdated Show resolved Hide resolved
cli/command/telemetry.go Outdated Show resolved Hide resolved
cli/command/telemetry_docker.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2024

Codecov Report

Merging #4940 (3bc46b3) into master (b8d5454) will decrease coverage by 0.22%.
The diff coverage is 10.86%.

❗ Current head 3bc46b3 differs from pull request most recent head efd82e1. Consider uploading reports for the commit efd82e1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4940      +/-   ##
==========================================
- Coverage   61.19%   60.97%   -0.22%     
==========================================
  Files         294      295       +1     
  Lines       20538    20625      +87     
==========================================
+ Hits        12568    12576       +8     
- Misses       7076     7154      +78     
- Partials      894      895       +1     

cmd/docker/docker.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
@laurazard
Copy link
Member

@jsternberg's PR #4889 just got merged 🥳

Can you rebase and see if everything's fine @krissetto?

@krissetto krissetto force-pushed the otel-init branch 4 times, most recently from 0a865b9 to f0162c8 Compare March 26, 2024 14:46
@laurazard
Copy link
Member

laurazard commented Mar 27, 2024

Just so I don't forget this, can you get code into telemetry.go to accept a OTEL_EXPORTER_OTLP_ENDPOINT env var in this PR too @krissetto?

@krissetto krissetto marked this pull request as ready for review March 27, 2024 14:48
@krissetto krissetto changed the title 🚧 Initial otel implementation 🚧 Initial otel implementation Mar 27, 2024
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

From my side I think this is good. I'll approve so long to revert my "requested changes" :)

cmd/docker/docker.go Outdated Show resolved Hide resolved
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 🥳 looks like there's a couple of linting errors, but other than that I'm happy with the current state of the PR.

@laurazard
Copy link
Member

@jsternberg wanna take a final look? :')

Copy link
Contributor

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

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

A few more comments but this mostly looks good to me.

cli/command/telemetry.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
cli/command/telemetry_utils.go Outdated Show resolved Hide resolved
Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
…testing purposes

Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
@laurazard laurazard merged commit 400a8bb into docker:master Mar 28, 2024
88 checks passed
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.

None yet

6 participants