-
Notifications
You must be signed in to change notification settings - Fork 2
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: introduce organic keyword audit (SITES-19317) #112
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good so far, with some observations and questions below
@@ -30,6 +31,7 @@ const HANDLERS = { | |||
'lhs-desktop': lhs, | |||
404: notfound, | |||
'broken-backlinks': backlinks, | |||
'organic-keywords': organicKeywords, |
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.
For all of the keys here, we could probably reuse the types exported from the audit model
src/support/ahrefs-client.js
Outdated
], | ||
}; | ||
|
||
return this.sendRequest('/site-explorer/organic-keywords', queryParams); |
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.
What is the cost per row for this query?
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.
Base const: 50
keyword: 1
best_position: 1
best_position_prev: 1
best_position_diff: 1
sum_traffic: 10
Total: 64 units
Should we document this somewhere?
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.
Good idea, maybe add it to the js doc of the function?
src/support/ahrefs-client.js
Outdated
const today = new Date(); | ||
|
||
const queryParams = { | ||
country: 'au', |
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.
Is au
country applicable to all sites?
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.
no, this is site specific. @alinarublea just showed me how I can integrate the alert config, I will integrate it there
return notFound('Site not found'); | ||
} | ||
|
||
const auditConfig = site.getAuditConfig(); |
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.
We should run the audit only for sites that are live
src/organic-keywords/handler.js
Outdated
} | ||
|
||
const ahrefsAPIClient = AhrefsAPIClient.createFrom(context); | ||
const response = await ahrefsAPIClient.getOrganicKeywords(site.getBaseURL(), auditContext); |
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.
How do you query and get results from ahrefs, using the www or non-www version of the site URL?
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.
shouldn't it be exactly the same as the saved base url? I tested in ahrefs and if it's not the same it does not return the right results
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've checked ahrefs UI with bamboohr.com
and it returns no results, whereas www.bamboohr.com does.
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.
bamboohr.com
also forwards to www.bamboohr.com
. Shouldn't we have www.bamboohr.com
as a baseURL in that case?
sum_traffic: 123, | ||
best_position: 1, | ||
best_position_prev: 1, | ||
best_position_diff: 0, |
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.
This is something we could easily compute ourselves. How much does it cost to get this field for every keyword every time we run the audit for a site?
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.
it's just 1 unit, so I thought it might save us some performance. Is it worth saving one unit?
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.
1 unit per row x 15 rows x ~hundred(s) live sites (in later phases) x number of audits scheduled or triggered per month
If we can compute it as easy as this, I'd say we save this 1 unit for fields that we cannot compute ourselves in other ahrefs audit types.
This PR will trigger a minor release when merged. |
# Conflicts: # package-lock.json # package.json
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 12 13 +1
Lines 1306 1414 +108
==========================================
+ Hits 1306 1414 +108 ☔ View full report in Codecov by Sentry. |
API trigger implemented here: adobe/spacecat-api-service#130
Post Processor implemented here: adobe/spacecat-audit-post-processor#65