Skip to content

Conversation

LadyPython
Copy link

@LadyPython LadyPython commented Aug 26, 2025

Context

If present, use eventProxiedTime header. Otherwise, use record.timestamp().

eventProxiedTime header should be replaced by standardized header name when available.

@Copilot Copilot AI review requested due to automatic review settings August 26, 2025 10:53
Copy link

@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 modifies latency calculations in Kafka's MirrorSourceTask to use a custom eventProxiedTime header when available, falling back to the record's timestamp. This change aims to provide more accurate latency measurements by accounting for additional processing time indicated by the header.

  • Adds logic to check for and parse the eventProxiedTime header for latency calculations
  • Includes error handling for header parsing failures with fallback to record timestamp
  • Imports StandardCharsets for proper string encoding

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

try {
correctedTimestamp = Long.parseLong(new String(eventProxiedTimeHeader.value(), StandardCharsets.UTF_8));
} catch (Exception e) {
log.error("Error parsing eventProxiedTime header value '{}' -- using record timestamp instead.", eventProxiedTimeHeader.value(), e);
Copy link
Preview

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The error message logs the raw byte array from eventProxiedTimeHeader.value(), which will not display the actual string value. Consider logging the parsed string instead for better debugging.

Suggested change
log.error("Error parsing eventProxiedTime header value '{}' -- using record timestamp instead.", eventProxiedTimeHeader.value(), e);
log.error("Error parsing eventProxiedTime header value '{}' -- using record timestamp instead.", new String(eventProxiedTimeHeader.value(), StandardCharsets.UTF_8), e);

Copilot uses AI. Check for mistakes.

@LadyPython LadyPython changed the base branch from trunk to 4.0-connect_sni_host_check_disable August 26, 2025 11:13
@LadyPython LadyPython changed the base branch from 4.0-connect_sni_host_check_disable to trunk August 26, 2025 11:13
@wise-github-bot-app
Copy link

The approval(s) from tkornai do(es)n't fullfill the approvers requirements because:

  • The approver's cost centre, ENGPLA, maps to the PLATFORM business function. As the code that was changed is owned by ENGINEERING, this approval won't satisfy our separation of duties check. We'll need an additional approval from someone in ENGINEERING. This approval may still help satisfy other codeowner requirements.

@LadyPython LadyPython force-pushed the change-latency-calculations branch from b8cee59 to 7914712 Compare August 26, 2025 12:32
@wise-github-bot-app
Copy link

The approval(s) from tkornai do(es)n't fullfill the approvers requirements because:

  • The approver's cost centre, ENGPLA, maps to the PLATFORM business function. As the code that was changed is owned by ENGINEERING, this approval won't satisfy our separation of duties check. We'll need an additional approval from someone in ENGINEERING. This approval may still help satisfy other codeowner requirements.

@LadyPython LadyPython merged commit dbdb2c6 into 4.0-connect_sni_host_check_disable Aug 26, 2025
1 check passed
@LadyPython LadyPython deleted the change-latency-calculations branch August 26, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants