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

Do not set TraceProvider to global #5138

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

bogdandrutu
Copy link
Member

The reason is because soon we will have the config for the tracing/metrics providers in the main config (already started).
When this happens then we can have users "reconfigure" that via the watch API, and re-setting the global is not allowed in otel (also not restart friendly). Because of that we should break this behavior now, and not install the global.

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

@bogdandrutu bogdandrutu requested review from a team and jpkrohling April 1, 2022 19:53
@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #5138 (ed60a5c) into main (9d47500) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5138      +/-   ##
==========================================
+ Coverage   89.90%   89.94%   +0.03%     
==========================================
  Files         182      182              
  Lines       11080    11077       -3     
==========================================
+ Hits         9962     9963       +1     
+ Misses        892      889       -3     
+ Partials      226      225       -1     
Impacted Files Coverage Δ
service/collector.go 76.54% <ø> (-0.57%) ⬇️
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
model/internal/pdata/common.go 94.26% <0.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2e5601...ed60a5c. Read the comment docs.

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

I think this should already be a non-issue since the provider in the TelemetrySettings should be used by all components. Definitely good to eliminate this, though.

@codeboten codeboten added Skip Changelog PRs that do not require a CHANGELOG.md entry and removed Skip Changelog PRs that do not require a CHANGELOG.md entry labels Apr 2, 2022
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Probably worth a changelog in case this breaks anyone using the global provider.

The reason is because soon we will have the config for the tracing/metrics providers in the main config (already started).
When this happens then we can have users "reconfigure" that via the watch API, and re-setting the global is not allowed in otel (also not restart friendly). Because of that we should break this behavior now, and not install the global.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Member Author

@codeboten thanks for reminding me... I forgot to push the changes :))

@bogdandrutu bogdandrutu merged commit 10ae1ef into open-telemetry:main Apr 4, 2022
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Apr 4, 2022
…cs featuregate enabled

This PR also removes the set of the global MeterProvider, similarly with open-telemetry#5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Apr 4, 2022
…cs featuregate enabled

This PR also removes the set of the global MeterProvider, similarly with open-telemetry#5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Apr 4, 2022
…cs featuregate enabled

This PR also removes the set of the global MeterProvider, similarly with open-telemetry#5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Apr 4, 2022
…cs featuregate enabled

This PR also removes the set of the global MeterProvider, similarly with open-telemetry#5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Apr 4, 2022
…cs featuregate enabled

This PR also removes the set of the global MeterProvider, similarly with open-telemetry#5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit that referenced this pull request Apr 4, 2022
…cs featuregate enabled (#5146)

This PR also removes the set of the global MeterProvider, similarly with #5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
The reason is because soon we will have the config for the tracing/metrics providers in the main config (already started).
When this happens then we can have users "reconfigure" that via the watch API, and re-setting the global is not allowed in otel (also not restart friendly). Because of that we should break this behavior now, and not install the global.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
…cs featuregate enabled (open-telemetry#5146)

This PR also removes the set of the global MeterProvider, similarly with open-telemetry#5138.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu deleted the rmglobabl branch October 14, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants