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

Centralize init of Meter/TracerProviders #5067

Merged
merged 1 commit into from
May 14, 2024

Conversation

krissetto
Copy link
Contributor

@krissetto krissetto commented May 13, 2024

- What I did

  • Centralized init of Meter/TracerProviders
  • Changed Meter/TracerProvider funcs on DockerCli to return the providers rather than initialize them

- Description for the changelog

Centralize initialization of Meter/TracerProviders

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

@krissetto krissetto requested a review from laurazard May 13, 2024 12:01
@krissetto krissetto self-assigned this May 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

Attention: Patch coverage is 64.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 61.35%. Comparing base (6c70360) to head (02537ea).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5067      +/-   ##
==========================================
+ Coverage   61.09%   61.35%   +0.26%     
==========================================
  Files         298      298              
  Lines       20675    20685      +10     
==========================================
+ Hits        12631    12691      +60     
+ Misses       7147     7093      -54     
- Partials      897      901       +4     

@krissetto krissetto force-pushed the adjust-otel-utils branch 2 times, most recently from 9c8b62d to 0feb0dc Compare May 13, 2024 12:05
@krissetto krissetto marked this pull request as ready for review May 13, 2024 12:30
@krissetto krissetto requested a review from jsternberg May 13, 2024 12:30
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

cmd/docker/docker.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
cmd/docker/docker.go Outdated Show resolved Hide resolved
@laurazard laurazard requested a review from vvoland May 14, 2024 13:51
@laurazard
Copy link
Member

Chatted w/ @jsternberg, there's some other approaches that would be interesting to look at (take care of shutdown automatically in the CLI lifecycle), but he's okay with merging as-is for now to unblock releases and improving things later.

Copy link
Contributor

@vvoland vvoland 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 think we should have a log in case the typecast fails (so like panic, but without actually panicking), so we can be aware if our assumptions are broken with future OTEL updates.

Not a blocker if you think it's fine though.

cmd/docker/docker.go Outdated Show resolved Hide resolved
…e them. Initialize them during DockerCli struct init

Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah added this to the 27.0.0 milestone May 14, 2024
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