Skip to content

Commit b557931

Browse files
swatijha23swatijha23
committed
NR-480384: Resolve review comments
1 parent 5edd1a5 commit b557931

File tree

1 file changed

+69
-36
lines changed

1 file changed

+69
-36
lines changed

‎NewRelicVideoCore/src/main/java/com/newrelic/videoagent/core/tracker/NRVideoTracker.java‎

Lines changed: 69 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public NRVideoTracker() {
9393
contentRequestTimestamp = null;
9494
contentStartTimestamp = null;
9595
contentErrorTimestamp = null;
96-
startupPeriodAdTime = 0L;
96+
startupPeriodAdTime = null;
9797
hasContentStarted = false;
9898

9999
// Initialize time-weighted bitrate tracking
@@ -271,7 +271,12 @@ public Map<String, Object> getAttributes(String action, Map<String, Object> attr
271271
public void updatePlaytime() {
272272
if (playtimeSinceLastEventTimestamp > 0) {
273273
playtimeSinceLastEvent = System.currentTimeMillis() - playtimeSinceLastEventTimestamp;
274-
totalPlaytime += playtimeSinceLastEvent;
274+
try {
275+
totalPlaytime = Math.addExact(totalPlaytime, playtimeSinceLastEvent);
276+
} catch (ArithmeticException e) {
277+
NRLog.w("Total playtime accumulation overflow - resetting to prevent data corruption");
278+
totalPlaytime = playtimeSinceLastEvent;
279+
}
275280
playtimeSinceLastEventTimestamp = System.currentTimeMillis();
276281
} else {
277282
playtimeSinceLastEvent = 0L;
@@ -487,7 +492,12 @@ public void sendBufferEnd() {
487492
Map<String, Object> attributes = getAttributes(CONTENT_BUFFER_END, null);
488493
Object timeSinceBufferBegin = attributes.get("timeSinceBufferBegin");
489494
if (timeSinceBufferBegin instanceof Long && !bufferType.equals("initial")) {
490-
qoeTotalRebufferingTime += (Long) timeSinceBufferBegin;
495+
try {
496+
qoeTotalRebufferingTime = Math.addExact(qoeTotalRebufferingTime, (Long) timeSinceBufferBegin);
497+
} catch (ArithmeticException e) {
498+
NRLog.w("QoE rebuffering time accumulation overflow - resetting to prevent data corruption");
499+
qoeTotalRebufferingTime = (Long) timeSinceBufferBegin;
500+
}
491501
}
492502
sendVideoEvent(CONTENT_BUFFER_END);
493503
}
@@ -625,7 +635,7 @@ private Map<String, Object> calculateQOEKpiAttributes() {
625635
kpiAttributes.put("averageBitrate", timeWeightedAverage);
626636
} else if (qoeBitrateCount > 0) {
627637
// Fallback to simple average if time-weighted calculation is not available
628-
long averageBitrate = qoeBitrateSum / qoeBitrateCount;
638+
long averageBitrate = Math.round((double) qoeBitrateSum / qoeBitrateCount);
629639
kpiAttributes.put("averageBitrate", averageBitrate);
630640
}
631641

@@ -644,10 +654,6 @@ private Map<String, Object> calculateQOEKpiAttributes() {
644654
private void updateTimeWeightedBitrate(Long newBitrate) {
645655
long currentTime = System.currentTimeMillis();
646656

647-
// Safety check: ensure fields are initialized
648-
if (qoeTotalBitrateWeightedTime == null) qoeTotalBitrateWeightedTime = 0L;
649-
if (qoeTotalActiveTime == null) qoeTotalActiveTime = 0L;
650-
651657
// If we have a previous bitrate and timing, accumulate its weighted time
652658
if (qoeCurrentBitrate != null && qoeLastRenditionChangeTime != null && qoeCurrentBitrate > 0) {
653659
// Ensure valid timestamps
@@ -656,16 +662,23 @@ private void updateTimeWeightedBitrate(Long newBitrate) {
656662
if (segmentDuration > 0) {
657663
// Prevent overflow in multiplication
658664
if (qoeCurrentBitrate <= Long.MAX_VALUE / segmentDuration) {
659-
qoeTotalBitrateWeightedTime += qoeCurrentBitrate * segmentDuration;
660-
qoeTotalActiveTime += segmentDuration;
665+
try {
666+
qoeTotalBitrateWeightedTime = Math.addExact(qoeTotalBitrateWeightedTime, qoeCurrentBitrate * segmentDuration);
667+
qoeTotalActiveTime = Math.addExact(qoeTotalActiveTime, segmentDuration);
668+
} catch (ArithmeticException e) {
669+
// Overflow in time-weighted bitrate accumulation - reset to prevent future overflows
670+
NRLog.w("QoE bitrate accumulation overflow - resetting accumulated values to start fresh");
671+
qoeTotalBitrateWeightedTime = 0L;
672+
qoeTotalActiveTime = 0L;
673+
}
661674
}
662675
}
663676
}
664677
}
665678

666679
// Update current tracking values (accept null/zero values for reset scenarios)
667680
qoeCurrentBitrate = newBitrate;
668-
qoeLastRenditionChangeTime = (currentTime > 0) ? currentTime : System.currentTimeMillis();
681+
qoeLastRenditionChangeTime = currentTime;
669682
}
670683

671684
/**
@@ -675,10 +688,6 @@ private void updateTimeWeightedBitrate(Long newBitrate) {
675688
* @return Time-weighted average bitrate, or null if no data available
676689
*/
677690
private Long calculateTimeWeightedAverageBitrate() {
678-
// Safety check: ensure required fields are properly initialized
679-
if (qoeTotalBitrateWeightedTime == null) qoeTotalBitrateWeightedTime = 0L;
680-
if (qoeTotalActiveTime == null) qoeTotalActiveTime = 0L;
681-
682691
// Include current segment in calculation
683692
if (qoeCurrentBitrate != null && qoeLastRenditionChangeTime != null && qoeCurrentBitrate > 0) {
684693
long currentTime = System.currentTimeMillis();
@@ -691,17 +700,24 @@ private Long calculateTimeWeightedAverageBitrate() {
691700
if (currentSegmentDuration > 0) {
692701
// Prevent overflow in multiplication
693702
if (qoeCurrentBitrate <= Long.MAX_VALUE / currentSegmentDuration) {
694-
long totalWeightedTime = qoeTotalBitrateWeightedTime + (qoeCurrentBitrate * currentSegmentDuration);
695-
long totalTime = qoeTotalActiveTime + currentSegmentDuration;
696-
697-
if (totalTime > 0) {
698-
return totalWeightedTime / totalTime;
703+
try {
704+
long totalWeightedTime = Math.addExact(qoeTotalBitrateWeightedTime, qoeCurrentBitrate * currentSegmentDuration);
705+
long totalTime = Math.addExact(qoeTotalActiveTime, currentSegmentDuration);
706+
707+
if (totalTime > 0) {
708+
return Math.round((double) totalWeightedTime / totalTime);
709+
}
710+
} catch (ArithmeticException e) {
711+
// Overflow in time-weighted bitrate calculation - reset to prevent future overflows
712+
NRLog.w("QoE bitrate calculation overflow - resetting accumulated values to start fresh");
713+
qoeTotalBitrateWeightedTime = 0L;
714+
qoeTotalActiveTime = 0L;
699715
}
700716
}
701717
}
702718
// If current segment has zero duration, check if we have accumulated data
703719
else if (qoeTotalActiveTime > 0) {
704-
return qoeTotalBitrateWeightedTime / qoeTotalActiveTime;
720+
return Math.round((double) qoeTotalBitrateWeightedTime / qoeTotalActiveTime);
705721
}
706722
// If we have current bitrate but no accumulated time and zero segment duration,
707723
// return current bitrate as the average (single point average)
@@ -713,7 +729,7 @@ else if (qoeTotalActiveTime == 0 && currentSegmentDuration == 0) {
713729

714730
// Fallback to accumulated data only
715731
if (qoeTotalActiveTime != null && qoeTotalActiveTime > 0 && qoeTotalBitrateWeightedTime != null) {
716-
return qoeTotalBitrateWeightedTime / qoeTotalActiveTime;
732+
return Math.round((double) qoeTotalBitrateWeightedTime / qoeTotalActiveTime);
717733
}
718734

719735
return null; // No time-weighted data available
@@ -746,6 +762,33 @@ private void resetQoeMetrics() {
746762
qoeTotalActiveTime = 0L;
747763
}
748764

765+
/**
766+
* Reset QoE metrics but preserve error timestamps for startup time calculation.
767+
* Called after errors to clean metrics while keeping timing data for analysis.
768+
*/
769+
private void resetQoeMetricsPreservingTimestamps() {
770+
qoePeakBitrate = null;
771+
qoeHadPlaybackFailure = false;
772+
qoeTotalRebufferingTime = 0L;
773+
qoeBitrateSum = 0L;
774+
qoeBitrateCount = 0L;
775+
qoeLastTrackedBitrate = null; // Reset cache
776+
qoeStartupTime = null; // Reset cached startup time for new view session
777+
778+
// DO NOT reset startup time calculation fields - preserve for error analysis:
779+
// contentRequestTimestamp - PRESERVED
780+
// contentStartTimestamp - PRESERVED
781+
// contentErrorTimestamp - PRESERVED
782+
// startupPeriodAdTime - PRESERVED
783+
hasContentStarted = false; // Reset state flag
784+
785+
// Reset time-weighted bitrate fields
786+
qoeCurrentBitrate = null;
787+
qoeLastRenditionChangeTime = null;
788+
qoeTotalBitrateWeightedTime = 0L;
789+
qoeTotalActiveTime = 0L;
790+
}
791+
749792
/**
750793
* Send request event.
751794
*
@@ -787,6 +830,10 @@ public void sendError(int errorCode, String errorMessage) {
787830
qoeHadPlaybackFailure = true;
788831
}
789832
}
833+
834+
// Reset QoE metrics but preserve error timestamps for startup time calculation
835+
resetQoeMetricsPreservingTimestamps();
836+
790837
sendVideoErrorEvent(actionName, errAttr);
791838
}
792839

@@ -1165,8 +1212,6 @@ public void generateTimeSinceTable() {
11651212
addTimeSinceEntry(CONTENT_RENDITION_CHANGE, "timeSinceLastRenditionChange", "^CONTENT_RENDITION_CHANGE$");
11661213
addTimeSinceEntry(AD_RENDITION_CHANGE, "timeSinceLastAdRenditionChange", "^AD_RENDITION_CHANGE$");
11671214

1168-
addTimeSinceEntry(QOE_AGGREGATE, "timeSinceLastQoeAggregate", "^QOE_AGGREGATE$");
1169-
11701215
addTimeSinceEntry(AD_BREAK_START, "timeSinceAdBreakBegin", "^AD_BREAK_END$");
11711216

11721217
addTimeSinceEntry(AD_QUARTILE, "timeSinceLastAdQuartile", "^AD_QUARTILE$");
@@ -1250,11 +1295,6 @@ public void sendVideoEvent(String action, Map<String, Object> attributes) {
12501295
* @param processedAttributes Fully processed attributes including contentBitrate
12511296
*/
12521297
private void trackBitrateFromProcessedAttributes(String action, Map<String, Object> processedAttributes) {
1253-
// Fast filter: only track for events that can have bitrate information
1254-
if (!isContentBitrateEvent(action)) {
1255-
return;
1256-
}
1257-
12581298
Long currentBitrate = extractBitrateValue(processedAttributes.get("contentBitrate"));
12591299
if (currentBitrate == null || currentBitrate <= 0) {
12601300
return;
@@ -1272,13 +1312,6 @@ private void trackBitrateFromProcessedAttributes(String action, Map<String, Obje
12721312
updateQoeBitrateMetrics(currentBitrate, action);
12731313
}
12741314

1275-
/**
1276-
* Fast check if this action can contain bitrate information.
1277-
*/
1278-
private static boolean isContentBitrateEvent(String action) {
1279-
return CONTENT_HEARTBEAT.equals(action) || CONTENT_START.equals(action) ||
1280-
CONTENT_RENDITION_CHANGE.equals(action) || CONTENT_RESUME.equals(action);
1281-
}
12821315

12831316
/**
12841317
* Efficiently extract Long bitrate value from various numeric types.

0 commit comments

Comments
 (0)