-
Notifications
You must be signed in to change notification settings - Fork 520
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
Domain services log context #19000
Draft
SimonRichardson
wants to merge
10
commits into
juju:main
Choose a base branch
from
SimonRichardson:domain-services-log-context
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Domain services log context #19000
SimonRichardson
wants to merge
10
commits into
juju:main
from
SimonRichardson:domain-services-log-context
Conversation
This file contains 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
This is preliminary to rework the logger for the introduction of writing status history to the model loggers.
The log sink worker is now a logger, we can start removing the hacky creation of a log worker. It can just be a worker that is request, much like the domain services.
This verifies that we're actually creating the right model logger and that they correctly close down when we expect.
In order to create a logger with all the right information, we can start a model level logger with the correct name.
There is a race to initialise the logger everywhere, so we just need to create it when it's used.
The model worker manager use to create an ad-hoc logger context when it started, instead we can prove out this change by wiring that context directly with the central logger.
We don't need to double write the logsink twice when we start the model worker manager. The logsink is the logger context. This means we just need to request that and it has everything already setup. This means we can wind out the agent from the modelworkermanager and just use the id from the agent in the logsink and not in the modelworkermanager. We've got the same functionality, but nicely encapsulated inside of the logsink. Later we'll push the logsink into the domainservices and all this refactoring will become a lot cleaner.
We no longer need the modelworkermanager recordlogger. It wasn't even tested!
In order for the logsink worker to be used as a dependency to the domain services, we have to employ the usual trick of having it's own set of services that allows the worker to interact with the database with minimal set of methods. This is the same pattern we've used with the provider tracker and the objectstore. The logsink now has less dependencies and we don't need to access the model in the apiserver everytime an agent requests the logsink for a model. This is cached at the logsink worker level. It inverts the dependency... If in the future we ever want to remove the naming scheme on the log files, it's easy as it's all encapsulated.
We need to change the domain service interface to accomodate failure when getting a service. The logger might not be available to we need to be sure we can actually get it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Checklist
QA steps
Documentation changes
Links
Launchpad bug: https://bugs.launchpad.net/juju/+bug/
Jira card: JUJU-