-
Notifications
You must be signed in to change notification settings - Fork 15
feat: moved calculateCpcValue function to spacecat-shared-utils
#1048
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
|
This PR will trigger a minor release when merged. |
| const organicTrafficData = await getObjectFromKey(s3Client, bucketName, key, log); | ||
| if (!Array.isArray(organicTrafficData) || organicTrafficData.length === 0) { | ||
| log.warn(`Organic traffic data not available for ${siteId}. Using Default CPC value.`); | ||
| return DEFAULT_CPC_VALUE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead setting default and return
how about we let downstream decide default.
As there will be no identifier if cpc is 1.5 its because of default or its actual.
Like upstream should get something which shows we are unable to fetch with reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another issue to this, what if the consumer doesn't define the CPC value or misses it? I mean not just for the reports, but for the audits like meta-tags or cwv, which will eventually use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean not just for the reports, but for the audits like meta-tags or cwv, which will eventually use this?
We will add that in documentation, whenever null or any case will occur they have to handle themselves.
We will provide actual value if null they have to handle like what they have to do that time
Default or don't have to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay got it.
| const organicTrafficData = await getObjectFromKey(s3Client, bucketName, key, log); | ||
| if (!Array.isArray(organicTrafficData) || organicTrafficData.length === 0) { | ||
| log.warn(`Organic traffic data not available for ${siteId}. Using Default CPC value.`); | ||
| return DEFAULT_CPC_VALUE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an object return.
Success
{
"value" : number
}
Failed :
{
"reason" : "an error occurred",
"value" : 1.5
}
SITES-36715
Add CPC Calculation Utility
Summary
Adds a new utility function to calculate Cost Per Click (CPC) values from Ahrefs organic traffic data stored in S3.
Changes
Added
calculateCPCValuefunction inmetrics-store.jsAdded
getObjectFromKeyhelper ins3.jsAdded
DEFAULT_CPC_VALUEconstant (1.5)Updated exports in
index.jsandindex.d.tsPlease ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!