Skip to content

feat: Add timing/performance logging to Logger class#2

Open
tsadrakula wants to merge 1 commit intomainfrom
feature/logger-timing
Open

feat: Add timing/performance logging to Logger class#2
tsadrakula wants to merge 1 commit intomainfrom
feature/logger-timing

Conversation

@tsadrakula
Copy link
Owner

Add startTimer, endTimer, getTimingReport, and clearTimings methods for measuring and logging operation durations with formatted output.

Add startTimer, endTimer, getTimingReport, and clearTimings methods
for measuring and logging operation durations with formatted output.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@shyft-local-dev shyft-local-dev bot left a comment

Choose a reason for hiding this comment

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

⚠️ Code Review by CodeCamel

Score: 78/100

Good implementation of timing functionality with some minor issues around precision, error handling, and API design that could be improved.


7 issues found, 4 reported based on your settings.

*/
startTimer(label: string): void {
this.timings.set(label, {
label,

Choose a reason for hiding this comment

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

MAJOR (80% confidence)

Using Date.now() for timing measurements can be inaccurate due to system clock adjustments. Consider using performance.now() for more precise timing measurements, especially for performance monitoring.

this.warning(`Timer "${label}" was never started`);
return 0;
}

Choose a reason for hiding this comment

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

MINOR (90% confidence)

Redundant Date.now() call - you're calculating duration correctly but then overwriting entry.endTime with a new timestamp. Should be: entry.endTime = entry.startTime + duration;

*/
endTimer(label: string): number {
const entry = this.timings.get(label);
if (!entry) {

Choose a reason for hiding this comment

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

MINOR (70% confidence)

Returning 0 for non-existent timers could mask bugs. Consider throwing an error or returning null/undefined to make the error more explicit to calling code.

const mins = Math.floor(duration / 60000);
const secs = ((duration % 60000) / 1000).toFixed(1);
durationStr = `${mins}m ${secs}s`;
}

Choose a reason for hiding this comment

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

MINOR (80% confidence)

The endTimer method always logs to console regardless of verbose setting, while startTimer respects it. Consider making this behavior consistent - either both respect verbose or both always log.

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.

1 participant