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

feat: cm360 batching support #2651

Closed
wants to merge 6 commits into from
Closed

Conversation

aashishmalik
Copy link
Contributor

Description of the change

cm360 batching support

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (deddae1) 86.93% compared to head (d7ca5c7) 86.94%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2651      +/-   ##
===========================================
+ Coverage    86.93%   86.94%   +0.01%     
===========================================
  Files          605      605              
  Lines        28167    28223      +56     
  Branches      6703     6715      +12     
===========================================
+ Hits         24486    24539      +53     
- Misses        3348     3350       +2     
- Partials       333      334       +1     
Files Coverage Δ
src/v0/destinations/campaign_manager/config.js 100.00% <100.00%> (ø)
src/v0/destinations/campaign_manager/transform.js 92.81% <95.52%> (+5.43%) ⬆️
...v0/destinations/campaign_manager/networkHandler.js 20.00% <0.00%> (-3.34%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

requestJson.timestampMicros = requestJson.timestampMicros.toString();

const date = new Date(requestJson.timestampMicros);
let unixTimestamp = date.getTime();
Copy link
Collaborator

@krishna2020 krishna2020 Sep 25, 2023

Choose a reason for hiding this comment

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

Doesn't this always return in milliseconds only irrespective of input passed ?

Copy link
Contributor Author

@aashishmalik aashishmalik Sep 26, 2023

Choose a reason for hiding this comment

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

If input is 1668624722555000 (this is in microseconds, year 2022) , the date utility still considers it is in milliseconds and convert this to 054846-08-12T03:22:35.000Z (year 5000) , when we call getTime it return 1668624722555000, so doing no of digits check here.

Same case with 1668624722 it considers it in ms, and getTime return it in 1668624722, so need to multiply by 6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think one alternative thing we can do if they pass timestamp in properties, we can make it mandatory to pass it in microSeconds and get rid of this conversion at our end

Copy link
Contributor

Choose a reason for hiding this comment

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

function convertToMicroseconds(input) {
  const timestamp = Date.parse(input);

  if (!isNaN(timestamp)) {
    // If the input is a valid date string, timestamp will be a number
    if (input.includes("Z")) {
      // ISO 8601 date string with milliseconds
      return timestamp * 1000;
    } else {
      // ISO 8601 date string without milliseconds
      return timestamp * 1000000;
    }
  } else if (/^\d+$/.test(input)) {
    // If the input is a numeric string (assume microseconds or milliseconds)
    if (input.length <= 13) {
      // Length less than or equal to 13 indicates milliseconds
      return parseInt(input) * 1000;
    } else {
      // Otherwise, assume microseconds
      return parseInt(input);
    }
  } else {
    // Invalid input
    throw new Error("Invalid input format");
  }
}

// Example usage:
const input1 = "2023-10-11T12:34:56.789Z"; // ISO 8601 date string with milliseconds
const input2 = "1633956896789"; // Milliseconds
const input3 = "1633956896789000"; // Microseconds

console.log(convertToMicroseconds(input1)); // Output: 1633956896789000
console.log(convertToMicroseconds(input2)); // Output: 1633956896789000
console.log(convertToMicroseconds(input3)); // Output: 1633956896789000

This code defines the convertToMicroseconds function, which first checks if the input is a valid date string (ISO 8601 format). If it is, it checks whether the input has milliseconds or not and converts it accordingly. If the input is a numeric string, it checks its length to determine whether it's in milliseconds or microseconds. If the input doesn't match any of these formats, it throws an error for an invalid input format.
Source: chatgpt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

console.log(convertToMicroseconds("2022-11-17T00:22:02.903+05:30"));
o/p -->>1668624722903000000 (in nanosweconds)
will modify this a bit

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

76.0% 76.0% Coverage
12.1% 12.1% Duplication

@aashishmalik aashishmalik marked this pull request as ready for review September 28, 2023 18:03
@aashishmalik aashishmalik requested review from a team as code owners September 28, 2023 18:03
requestJson.timestampMicros = requestJson.timestampMicros.toString();

const date = new Date(requestJson.timestampMicros);
let unixTimestamp = date.getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

function convertToMicroseconds(input) {
  const timestamp = Date.parse(input);

  if (!isNaN(timestamp)) {
    // If the input is a valid date string, timestamp will be a number
    if (input.includes("Z")) {
      // ISO 8601 date string with milliseconds
      return timestamp * 1000;
    } else {
      // ISO 8601 date string without milliseconds
      return timestamp * 1000000;
    }
  } else if (/^\d+$/.test(input)) {
    // If the input is a numeric string (assume microseconds or milliseconds)
    if (input.length <= 13) {
      // Length less than or equal to 13 indicates milliseconds
      return parseInt(input) * 1000;
    } else {
      // Otherwise, assume microseconds
      return parseInt(input);
    }
  } else {
    // Invalid input
    throw new Error("Invalid input format");
  }
}

// Example usage:
const input1 = "2023-10-11T12:34:56.789Z"; // ISO 8601 date string with milliseconds
const input2 = "1633956896789"; // Milliseconds
const input3 = "1633956896789000"; // Microseconds

console.log(convertToMicroseconds(input1)); // Output: 1633956896789000
console.log(convertToMicroseconds(input2)); // Output: 1633956896789000
console.log(convertToMicroseconds(input3)); // Output: 1633956896789000

This code defines the convertToMicroseconds function, which first checks if the input is a valid date string (ISO 8601 format). If it is, it checks whether the input has milliseconds or not and converts it accordingly. If the input is a numeric string, it checks its length to determine whether it's in milliseconds or microseconds. If the input doesn't match any of these formats, it throws an error for an invalid input format.
Source: chatgpt

events.forEach((ev) => {
conversions.push(...ev.message.body.JSON.conversions);
metadata.push(ev.metadata);
if (ev.message.body.JSON.encryptionInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will override the encryptionInfo, is this expected?

const { destination, message } = events[0];
// Batch event into dest batch structure
events.forEach((ev) => {
conversions.push(...ev.message.body.JSON.conversions);
Copy link
Contributor

Choose a reason for hiding this comment

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

you could keep this ev.message.body.JSON in a variable and reuse it instead of typing long strings

@devops-github-rudderstack devops-github-rudderstack deleted the fix.cm360-timestamp branch December 27, 2023 01:24
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.

5 participants