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

[FDS-2386] Global client caching, Telemetry auto instruementation #1129

Merged
merged 6 commits into from
Sep 19, 2024

Conversation

BryanFauble
Copy link
Contributor

Problem:

  1. I found some more places where internally in the client we were not propagating along the synapse_client to further functions. When I started to disable client caching it was causing a few integration tests within schematic to fail (Note: The code that caused this was only present in an integration test, and not present in any code hit during a production request to schematic).
  2. There was a lot of code added to handle otel context propagation to threads in the threading library.
  3. When the client is used in the context of a multi-user environment it is very easy to not set cache_client = False when creating a new Synapse object. As a result a client can unintentionally be cached.

Solution:

  1. Adding a global settings to disable client caching in the Synapse class
  2. Adding a global setting to enable otel instrumentation in the client.
  3. Using the otel auto instrumentation libraries to handle creating traces for several libraries.
  4. I updated all of the integration tests to not use any cache Synapse class instance, and instead everything needs to pass it along - This will be used to verify the code is correctly propagating the instance along when it needs to.

Testing:

  1. I am testing this actively with Schematic, along with integration tests that I verified ran locally/in GH.

@BryanFauble BryanFauble requested a review from a team as a code owner September 13, 2024 21:24
@@ -1025,6 +1021,7 @@ async def get_async(
if self.path and os.path.isfile(self.path)
else self.path,
md5=self.content_md5,
synapse_client=syn,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the spots that prompted me to make this change

and (existing_folder := await Folder(id=existing_folder_id).get_async())
and (
existing_folder := await Folder(id=existing_folder_id).get_async(
synapse_client=synapse_client
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the spots that prompted me to make this change

@@ -338,6 +336,7 @@ async def get_async(
await get_from_entity_factory(
entity_to_update=self,
synapse_id_or_path=entity_id,
synapse_client=synapse_client,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the spots that prompted me to make this change

),
)

new_folder_id = source_and_destination.get(self.id, None)
if not new_folder_id:
raise SynapseError("Failed to copy folder.")
folder_copy = await (
await Folder(id=new_folder_id).get_async()
await Folder(id=new_folder_id).get_async(synapse_client=syn)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the spots that prompted me to make this change

),
)

new_project_id = source_and_destination.get(self.id, None)
if not new_project_id:
raise SynapseError("Failed to copy project.")
project_copy = await (
await Project(id=new_project_id).get_async()
await Project(id=new_project_id).get_async(synapse_client=syn)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the spots that prompted me to make this change

synapseclient/client.py Outdated Show resolved Hide resolved
),
)

new_folder_id = source_and_destination.get(self.id, None)
if not new_folder_id:
raise SynapseError("Failed to copy folder.")
folder_copy = await (
await Folder(id=new_folder_id).get_async()
await Folder(id=new_folder_id).get_async(synapse_client=syn)
).sync_from_synapse_async(
download_file=False,
synapse_client=synapse_client,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be synapse_client=syn like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They end up working the exact same way because of how syn = Synapse.get_client(synapse_client=synapse_client) is used. However, I made a change here to make this consistent

Copy link
Collaborator

@jaymedina jaymedina left a comment

Choose a reason for hiding this comment

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

One last minor comment, apart from that everything looks good pending a passing build🔥

@BryanFauble BryanFauble merged commit 560c369 into develop Sep 19, 2024
23 checks passed
@BryanFauble BryanFauble deleted the fds-2386-allow-global-client-cache-disable branch September 19, 2024 20:13
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