-
Notifications
You must be signed in to change notification settings - Fork 363
Add logging of service metadata during instance create/update/delete #4386
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
base: main
Are you sure you want to change the base?
Conversation
…in service instance creation/update/deletion
7a5a766
to
cd65bf7
Compare
3a16543
to
e7b6273
Compare
72e4b4b
to
1ebd8ae
Compare
6a3b167
to
511e64b
Compare
"Updating managed service instance with name '#{service_instance.name}' " \ | ||
"using service plan '#{service_instance.service_plan.name}' " \ | ||
"from service offering '#{service_instance.service_plan.service.label}' " \ | ||
"provided by broker '#{service_instance.service_plan.service.service_broker.name}'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested this, a database query to fetch broker information was executed.
It seems as if service_instance.reload
(here) removes the associated objects (or at least some of them).
So for the update_managed
part, eager loading does not have the expected benefits.
Additional question: can the service plan be updated? This would even make the eager loading completely obsolete in such a case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service plan can be updated, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch with the reload! I removed the reload now and use a copy of the service instance instead for verifying the updated object, to ensure that follow-up preflight checks run with the original service instance object.
To your question: Yes, the plan is updatable. But only for the same service offering -> so eager loading the service and broker still provides a benefit because those will never change for a service instance.
I compared the number of DB queries in main with the ones from this PR:
Scenario/codebase | main | PR |
---|---|---|
create | 73 | 71 |
update | 95 | 91 |
delete | 48 | 48 |
-> even though we are fetching more information for the logs, this will reduce the number of queries for create/update cases and stays the same for the delete case.
8b89063
to
e4dbd87
Compare
Reloading the service_instance just ensures that follow-up preflight checks run with the original service instance values. This can also be achieved by applying changes on a copy of the object. With that, we don't loose eagerly loaded associations and save a few DB queries.
e4dbd87
to
84ed291
Compare
Inform operators which service plan, service offering and broker is involved in service instance creation/update/deletion
A short explanation of the proposed change:
Add names of service plan, service offering and broker as logging information for service instance creation/update/deletion.
This change updates fetch_writable_service_instance to eagerly load associated service_plan, service, and service_broker relationships for managed service instances. This avoids N+1 queries when accessing these associations later in the request lifecycle.
An explanation of the use cases your change solves
This change improves operator visibility by logging service plan names, offerings, and broker names during service instance creation, update, and deletion.
Previously, logs only included GUIDs or IDs, requiring manual lookups or customer input to identify the service involved. With this change, operators can directly see which service is affected, making it easier to diagnose issues and support users/customers more efficiently.
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
main
branchI have run all the unit tests using
bundle exec rake
I have run CF Acceptance Tests