Skip to content

Initial otel implementation - #4940

Merged
laurazard merged 3 commits into
docker:masterfrom
krissetto:otel-init
Mar 28, 2024
Merged

Initial otel implementation#4940
laurazard merged 3 commits into
docker:masterfrom
krissetto:otel-init

Conversation

@krissetto

@krissettokrissetto commented Mar 13, 2024

Copy link
Copy Markdown
Contributor

- 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)

@krissettokrissetto changed the title Initial otel implementation🚧 Initial otel implementation 🚧Mar 13, 2024
Comment threadvendor/github.com/go-logr/logr/funcr/funcr.go Fixed
Comment threadcli/command/telemetry.go
Comment threadcli/command/telemetry.go
Comment threadcli/command/telemetry.go Outdated
Comment threadcli/command/telemetry_docker.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcmd/docker/docker.go Outdated
@codecov-commenter

codecov-commenter commented Mar 16, 2024

Copy link
Copy Markdown

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 

Comment threadcmd/docker/docker.go Outdated
Comment threadcmd/docker/docker.go Outdated
Comment threadcli/command/telemetry_utils.go
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils_test.go Outdated
Comment threadcli/command/telemetry_utils_test.go Outdated
@laurazard

Copy link
Copy Markdown
Member

@jsternberg's PR #4889 just got merged 🥳

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

@krissetto
krissettoforce-pushed the otel-init branch 4 times, most recently from 0a865b9 to f0162c8CompareMarch 26, 2024 14:46
@laurazard

laurazard commented Mar 27, 2024

Copy link
Copy Markdown
Member

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?

Comment threadcli/command/telemetry_docker.go
@krissetto
krissetto marked this pull request as ready for review March 27, 2024 14:48
@krissettokrissetto changed the title 🚧 Initial otel implementation 🚧Initial otel implementationMar 27, 2024
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go
Comment threadcmd/docker/docker.go Outdated

@BenehikoBenehiko left a comment

Copy link
Copy Markdown
Member

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" :)

Comment threadcmd/docker/docker.go Outdated

@laurazardlaurazard left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

@jsternberg wanna take a final look? :')

@jsternbergjsternberg left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadcli/command/telemetry.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
Comment threadcli/command/telemetry_utils.go Outdated
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:masterMar 28, 2024
@thaJeztahthaJeztah added this to the 26.1.0 milestone Sep 18, 2024
Sign up for freeto 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.

7 participants

@krissetto@codecov-commenter@laurazard@jsternberg@Benehiko@github-advanced-security@thaJeztah