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

Kdesktop 1264 implement sentry transaction. #403

Draft
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

herve-er
Copy link
Contributor

@herve-er herve-er commented Nov 22, 2024

PerformanceTrace (PTrace) Integration for Enhanced Monitoring 🚀

The PerformanceTrace (PTrace) system is designed to track the execution time of various parts of our codebase and send performance data to Sentry for analysis. This enables monitoring, troubleshooting, and optimization the application.


How PerformanceTrace Works 📜

Manual PerformanceTrace (Not Recommended in Most Cases 🛑 )

In sentryhandler.h, the enum PTraceName defines all the traceable steps, such as:

  • PTraceName::Sync
  • PTraceName::UpdateDetection1
  • PTraceName::UpdateUnsyncedList, and others.

To minimize the impact in codebase readability due to performance monitoring, pTrace details are defined in the constructor of the SentryHandler::PTraceInfo structure.

Although SentryHandler::PTraceInfo is used exclusively within SentryHandler, it must be updated whenever a new trace is added. This structure provides the following:

  • Readable titles and descriptions for display on the Sentry dashboard.
  • Automatic management of parent/child relationships between PTraces.

Example implementation in the constructor:

SentryHandler::PTraceInfo::PTraceInfo(SentryHandler::PTraceName pTraceName) {
    _pTraceName = pTraceName;
    switch (_pTraceName) {
        case SentryHandler::PTraceName::Sync:
            _pTraceTitle = "Synchronization";
            _pTraceDescription = "Handles synchronization process.";
            break;
        case SentryHandler::PTraceName::UpdateDetection1:
            _pTraceTitle = "Update Detection 1";
            _pTraceDescription = "Computes file system operations.";
            _parentPTraceName = SentryHandler::PTraceName::Sync;
            break;
        case SentryHandler::PTraceName::UpdateUnsyncedList:
            _pTraceTitle = "Update Unsynced List";
            _pTraceDescription = "Updates the list of unsynced items.";
            _parentPTraceName = SentryHandler::PTraceName::UpdateDetection1;
            break;
        // Additional cases...
    }
}

This setup enables child PTraces to be started automatically as children of their respective parent traces without requiring explicit management of parent IDs by the caller (see below).

Example of Manual PTrace Management:

void synchronize() {
    SentryHandler::startPTrace(SentryHandler::PTraceName::Sync, syncDbId());
    computeFsOp();
    SentryHandler::stopPTrace(SentryHandler::PTraceName::Sync, syncDbId());
}

void computeFsOp() {
    auto pTraceId = SentryHandler::startPTrace(SentryHandler::PTraceName::UpdateDetection1, syncDbId());  
    updateUnsyncedList();
    SentryHandler::stopPTrace(pTraceId); // Same as SentryHandler::stopPTrace(SentryHandler::PTraceName::UpdateDetection1, syncDbId()); 
}

void updateUnsyncedList() {
    auto pTraceId = SentryHandler::startPTrace(SentryHandler::PTraceName::UpdateUnsyncedList, syncDbId());  
    // Perform operations...
    SentryHandler::stopPTrace(pTraceId); // Same as SentryHandler::stopPTrace(SentryHandler::PTraceName::UpdateUnsyncedList, syncDbId());  
}

Scoped PerformanceTrace (Recommended in Most Cases ✅ 🦺 )

The SentryHandler::ScopedPTrace class simplifies performance tracing by ensuring that traces are always stopped properly, avoiding:

  1. Memory leaks for orphaned transactions.
  2. Discarded traces for parented transactions (automatically deleted when the parent terminates but not send to sentry).

When a SentryHandler::ScopedPTrace object is destroyed, its associated trace is automatically stopped.

Constructor Options:

  1. Start a new trace using a PTraceName and a syncDbId.
  2. Attach to an existing trace using its pTraceId.

Optional: A manualStopExpected flag (default: false). If set to true, the trace must be stopped manually using stop() before the object is destroyed. Otherwise, the trace is marked as aborted, which is helpful for identifying problematic steps on the Sentry dashboard.

Example Usage:

  1. Without Manual Stop:
void func1() {
    auto perfMonitor = SentryHandler::ScopedPTrace(SentryHandler::PTraceName::func1, syncDbId());
    // Perform operations...
}
  1. With Manual Stop:
void func2() {
    auto perfMonitor = SentryHandler::ScopedPTrace(SentryHandler::PTraceName::func2, syncDbId(), true);
    if (!foo()) {
        // On failure, the trace is marked as aborted.
        return;
    }
    perfMonitor.stop(); // Explicit stop on success.
}

Additional Features:

  • SentryHandler::ScopedPTrace::stopAndStart Method: Stops the current trace and starts a new one with the provided parameters (same as ScopedPTrace constructor).
  • SentryHandler::ScopedPTrace::stop(true) can be call with the aborted set to true wich will force the pTrace to an aborted state.

Tracing Outside Synchronization

A PTrace or ScopedPTrace can be started outside the synchronization context by passing -1 for the syncDbId argument.


⚠️ Limitations

  1. A PTraceName can only be started once per syncDbId. (asserted in debug mode, ignored in release).

Sentry Dashboard 📊

The Sentry dashboard automatically processes PTraces to provide meaningful insights:

Trace Breakdown

Example: Breakdown of a specific sync operation.
Trace Breakdown

Performance Statistics

For each PTrace, we get metrics like median time (P50), P75, and P90 durations, along with other insights:
Performance Stats


Benefits of PTrace

This system provides invaluable data for:

  1. Tracking performance during the upcoming optimization phase.
  2. Identifying and addressing regressions in performance.

@herve-er herve-er requested a review from a team as a code owner November 22, 2024 11:57
@herve-er herve-er marked this pull request as draft November 22, 2024 12:46
@herve-er herve-er marked this pull request as ready for review November 22, 2024 15:15
@herve-er herve-er marked this pull request as draft November 25, 2024 06:00
@herve-er herve-er marked this pull request as ready for review November 25, 2024 07:24
@herve-er herve-er added this to the 3.6.8 milestone Nov 26, 2024
Comment on lines +303 to +304
pTraceId startTransaction(const std::string &name, const std::string &description);
pTraceId startSpan(const std::string &name, const std::string &description, const pTraceId &parentId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you briefly explain in comment what in a transaction and what is a span in Sentry context?

@@ -358,16 +367,46 @@ void SyncPalWorker::initStep(SyncStep step, std::shared_ptr<ISyncWorker> (&worke
}
}

SentryHandler::PTraceName SyncPalWorker::setpToPTraceName(SyncStep step) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this could be defined in SentryHandler instead to avoid increasing the size of SyncPalWorker class

Comment on lines 657 to 659
if (perfMonitorCounter % 1000 == 0) {
perfMonitor.stopAndStart(SentryHandler::PTraceName::LFSO_ExploreItem, syncDbId(), true);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we delegate this logic to the SentryHandler::ScopedPTrace class? The goal should always to add as less as possible into the caller method. LocalFileSystemObserverWorker is already quite complicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about this: 0a60107

Comment on lines 335 to 337
if (itemCount % 1000 == 0) {
sentryTransaction.stopAndStart(SentryHandler::PTraceName::RFSO_ExploreItem, syncDbId(), true);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as 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.

@herve-er herve-er marked this pull request as draft November 27, 2024 14:42
Copy link

sonarcloud bot commented Nov 27, 2024

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.

2 participants