Skip to content

Conversation

trask
Copy link
Member

@trask trask commented Jul 8, 2025

No description provided.

@trask trask marked this pull request as ready for review July 14, 2025 04:00
@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 04:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a race condition in the live metrics telemetry collector by introducing a read–write lock around the core counter operations.

  • Adds a ReadWriteLock to synchronize getAndRestart, peek, and per-item add logic.
  • Refactors helper methods to accept a Counters instance rather than fetching it internally.
  • Introduces early type and null checks to skip locking when no counters are available or for unsupported telemetry.
Comments suppressed due to low confidence (2)

sdk/monitor/azure-monitor-opentelemetry-autoconfigure/src/main/java/com/azure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseDataCollector.java:62

  • [nitpick] The field name lock is very generic; consider renaming it to something like countersLock or rwCountersLock to clarify its purpose.
    private final ReadWriteLock lock = new ReentrantReadWriteLock();

sdk/monitor/azure-monitor-opentelemetry-autoconfigure/src/main/java/com/azure/monitor/opentelemetry/autoconfigure/implementation/quickpulse/QuickPulseDataCollector.java:94

  • This method still uses synchronized instead of the new ReadWriteLock, causing mixed locking strategies that could lead to deadlock. Consider switching it to lock.readLock().lock()/unlock() or removing the synchronized keyword entirely.
    synchronized QuickPulseStatus getQuickPulseStatus() {

@trask trask enabled auto-merge (squash) July 22, 2025 16:59
@trask trask merged commit c5b5b7c into main Jul 22, 2025
17 checks passed
@trask trask deleted the live-metrics-race branch July 22, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monitor - Autoconfigure Monitor OpenTelemetry Autoconfigure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants