Skip to content

refactor(treewide): reorder statements to improve speed #953

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+31 −34
Open

refactor(treewide): reorder statements to improve speed #953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions tux/cog_loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
from tux.utils.config import CONFIG
from tux.utils.sentry import safe_set_name, span, start_span, transaction

SENTRY_INITIALIZED: bool = sentry_sdk.is_initialized()
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): SENTRY_INITIALIZED is set at import time, which may not reflect runtime state.

If sentry_sdk may be initialized after import, caching its state could cause incorrect behavior. Consider calling sentry_sdk.is_initialized() directly unless initialization is guaranteed to be static.



class CogLoadError(Exception):
"""Raised when a cog fails to load."""
Expand Down Expand Up @@ -90,7 +92,7 @@
cog_name = path.stem

# Add span tags for the current cog
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("cog.name", cog_name)
current_span.set_tag("cog.path", str(path))

Expand All @@ -101,7 +103,7 @@
# Convert path to module format (e.g., tux.cogs.admin.dev)
module = f"tux.{str(relative_path).replace('/', '.').replace('\\', '.')[:-3]}"

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("cog.module", module)

# Check if this module or any parent module is already loaded
Expand All @@ -112,7 +114,7 @@
check_module = ".".join(module_parts[:i])
if check_module in self.bot.extensions:
logger.warning(f"Skipping {module} as {check_module} is already loaded")
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("cog.status", "skipped")
current_span.set_tag("cog.skip_reason", "already_loaded")
current_span.set_data("already_loaded_module", check_module)
Expand All @@ -124,15 +126,15 @@
self.load_times[module] = load_time

# Add telemetry data to span
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("cog.status", "loaded")
current_span.set_data("load_time_ms", load_time * 1000)
current_span.set_data("load_time_s", load_time)

logger.debug(f"Successfully loaded cog {module} in {load_time * 1000:.0f}ms")

except Exception as e:
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_status("internal_error")
current_span.set_tag("cog.status", "failed")
current_span.set_data("error", str(e))
Expand Down Expand Up @@ -173,7 +175,7 @@
return

# Add basic info for the group
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("cog_count", len(cogs))

if categories := {cog.parent.name for cog in cogs if cog.parent}:
Expand All @@ -188,7 +190,7 @@
success_count = len([r for r in results if not isinstance(r, Exception)])
failure_count = len(results) - success_count

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("load_time_s", end_time - start_time)
current_span.set_data("success_count", success_count)
current_span.set_data("failure_count", failure_count)
Expand All @@ -200,14 +202,14 @@

async def _process_single_file(self, path: Path) -> None:
"""Process a single file path."""
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("path.is_dir", False)
if await self.is_cog_eligible(path):
await self._load_single_cog(path)

async def _process_directory(self, path: Path) -> None:
"""Process a directory of cogs."""
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("path.is_dir", True)

# Collect and sort eligible cogs by priority
Expand All @@ -216,7 +218,7 @@
]
cog_paths.sort(key=lambda x: x[0], reverse=True)

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("eligible_cog_count", len(cog_paths))

# Priority groups info for observability
Expand Down Expand Up @@ -254,7 +256,7 @@
The path to the directory containing cogs.
"""
# Add span context
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("cog.path", str(path))

try:
Expand All @@ -268,7 +270,7 @@
path_str = path.as_posix()
logger.error(f"An error occurred while processing {path_str}: {e}")

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_status("internal_error")
current_span.set_data("error", str(e))
current_span.set_data("traceback", traceback.format_exc())
Expand All @@ -286,22 +288,22 @@
The name of the folder containing the cogs.
"""
# Add span info
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("cog.folder", folder_name)
# Use safe_set_name instead of direct set_name call
safe_set_name(current_span, f"Load Cogs: {folder_name}")

start_time = time.perf_counter()
cog_path: Path = Path(__file__).parent / folder_name

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("full_path", str(cog_path))

try:
await self.load_cogs(path=cog_path)
load_time = time.perf_counter() - start_time

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("load_time_s", load_time)
current_span.set_data("load_time_ms", load_time * 1000)

Expand All @@ -311,12 +313,12 @@
# Log individual cog load times for performance monitoring
slow_threshold = 1.0 # seconds
if slow_cogs := {k: v for k, v in self.load_times.items() if v > slow_threshold}:
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("slow_cogs", slow_cogs)
logger.warning(f"Slow loading cogs (>{slow_threshold * 1000:.0f}ms): {slow_cogs}")

except Exception as e:
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_status("internal_error")
current_span.set_data("error", str(e))
current_span.set_data("traceback", traceback.format_exc())
Expand All @@ -335,12 +337,12 @@
bot : commands.Bot
The bot instance.
"""
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("bot.id", bot.user.id if bot.user else "unknown")

start_time = time.perf_counter()
cog_loader = cls(bot)

if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_tag("bot.id", bot.user.id if bot.user else "unknown")

Check warning on line 344 in tux/cog_loader.py

View check run for this annotation

Codecov / codecov/patch

tux/cog_loader.py#L344 

Added line #L344 was not covered by tests

try:
# Load handlers first (they have highest priority)
with start_span("cog.load_handlers", "Load handler cogs"):
Expand All @@ -356,7 +358,7 @@

total_time = time.perf_counter() - start_time

if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_data("total_load_time_s", total_time)
current_span.set_data("total_load_time_ms", total_time * 1000)

Expand All @@ -367,7 +369,7 @@
logger.info(f"Total cog loading time: {total_time * 1000:.0f}ms")

except Exception as e:
if sentry_sdk.is_initialized() and (current_span := sentry_sdk.get_current_span()):
if SENTRY_INITIALIZED and (current_span := sentry_sdk.get_current_span()):
current_span.set_status("internal_error")
current_span.set_data("error", str(e))
current_span.set_data("traceback", traceback.format_exc())
Expand Down
7 changes: 4 additions & 3 deletions tux/cogs/fun/xkcd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@
The ID of the xkcd comic to search for.
"""

if comic_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): The logic for handling comic_id has been inverted, which may affect behavior for comic_id=0.

The updated logic prevents access to comic 0 if it's valid. Explicitly check for None to allow comic_id=0.

await self.specific(ctx, comic_id)
else:
if not comic_id:
await ctx.send_help("xkcd")
return

Check warning on line 40 in tux/cogs/fun/xkcd.py

View check run for this annotation

Codecov / codecov/patch

tux/cogs/fun/xkcd.py#L40 

Added line #L40 was not covered by tests

await self.specific(ctx, comic_id)

Check warning on line 42 in tux/cogs/fun/xkcd.py

View check run for this annotation

Codecov / codecov/patch

tux/cogs/fun/xkcd.py#L42 

Added line #L42 was not covered by tests

@xkcd.command(
name="latest",
Expand Down
12 changes: 3 additions & 9 deletions tux/cogs/info/avatar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,22 +75,16 @@
member : discord.Member
The member to get the avatar of.
"""
if member is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): The function now only handles discord.Interaction sources for members, potentially breaking support for other source types.

If this change is intentional, please document or enforce the restriction. Otherwise, it may break support for non-Interaction sources.

if isinstance(source, discord.Interaction) and member is not None:
guild_avatar = member.guild_avatar.url if member.guild_avatar else None
global_avatar = member.avatar.url if member.avatar else None
files = [await self.create_avatar_file(avatar) for avatar in [guild_avatar, global_avatar] if avatar]

if files:
if isinstance(source, discord.Interaction):
await source.response.send_message(files=files)
else:
await source.reply(files=files)
await source.response.send_message(files=files)

Check warning on line 84 in tux/cogs/info/avatar.py

View check run for this annotation

Codecov / codecov/patch

tux/cogs/info/avatar.py#L84 

Added line #L84 was not covered by tests
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): No fallback for non-Interaction sources when sending avatar files.

If source can be something other than discord.Interaction, add a fallback or raise a clear error to avoid AttributeError.

message = "Member has no avatar."
if isinstance(source, discord.Interaction):
await source.response.send_message(content=message, ephemeral=True, delete_after=30)
else:
await source.reply(content=message, ephemeral=True, delete_after=30)
await source.response.send_message(content=message, ephemeral=True, delete_after=30)

Check warning on line 87 in tux/cogs/info/avatar.py

View check run for this annotation

Codecov / codecov/patch

tux/cogs/info/avatar.py#L87 

Added line #L87 was not covered by tests

elif isinstance(source, commands.Context):
member = await commands.MemberConverter().convert(source, str(source.author.id))
Expand Down
Loading