remove telemetry configuration hot reloading#1463
Merged
Conversation
Configuration hot reloading is not very useful for telemetry, and is the source of regular bugs that are hard to fix. This removes the support for configuration reloading entirely. Now, the router will reject a configuration reload with an error log if the telemetry configuration changed. A temporary tracing subscriber is created when the router is started, and is used until a subscriber can be created from the configuration and set as global subscriber. That global subscriber will then be used for all the requests and will not change for the entire life of the process.
This comment has been minimized.
This comment has been minimized.
Geal
commented
Aug 4, 2022
added 2 commits
August 4, 2022 16:11
Geal
commented
Aug 4, 2022
added 4 commits
August 5, 2022 16:35
When using the router as a library (example: in integration tests), we might want to use another tracing subscriber than the one created by default in the telemetry plugin. This adds a method to the telemetry plugin to directly provide a subscriber. It will then be modified to use layers for the telemetry exporters and spaceport, so it won't be possible to deactivate studio reportingt that way
bnjjj
reviewed
Aug 8, 2022
SimonSapin
reviewed
Aug 8, 2022
Contributor
SimonSapin
left a comment
SimonSapin
left a comment
There was a problem hiding this comment.
Please also remove this comment paragraph in router.rs
This must only be called in the context of Executable::builder() because it relies on custom logging setup to support hot reload.
SimonSapin
approved these changes
Aug 11, 2022
Contributor
|
Looks like |
bnjjj
approved these changes
Aug 11, 2022
o0Ignition0o
suggested changes
Aug 11, 2022
Contributor
o0Ignition0o
left a comment
o0Ignition0o
left a comment
There was a problem hiding this comment.
One big concern about the telemetry unpinning.
The rest looks good to me
added 3 commits
August 12, 2022 10:00
o0Ignition0o
approved these changes
Aug 12, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #1224
Configuration hot reloading is not very useful for telemetry, and is the
source of regular bugs that are hard to fix.
This removes the support for configuration reloading entirely. Now, the
router will reject a configuration reload with an error log if the
telemetry configuration changed.
A temporary tracing subscriber is created when the router is started, and
is used until a subscriber can be created from the configuration and set
as global subscriber. That global subscriber will then be used for all
the requests and will not change for the entire life of the process.