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: introduce organic keyword audit (SITES-19317) #112

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dzehnder
Copy link
Contributor

@dzehnder dzehnder commented Jan 30, 2024

API trigger implemented here: adobe/spacecat-api-service#130
Post Processor implemented here: adobe/spacecat-audit-post-processor#65

Copy link
Contributor

@iuliag iuliag left a 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,
Copy link
Contributor

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

],
};

return this.sendRequest('/site-explorer/organic-keywords', queryParams);
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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?

const today = new Date();

const queryParams = {
country: 'au',
Copy link
Contributor

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?

Copy link
Contributor Author

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();
Copy link
Contributor

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

}

const ahrefsAPIClient = AhrefsAPIClient.createFrom(context);
const response = await ahrefsAPIClient.getOrganicKeywords(site.getBaseURL(), auditContext);
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.
Screenshot 2024-01-30 at 14 51 32
Screenshot 2024-01-30 at 14 51 04

Copy link
Contributor Author

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link

This PR will trigger a minor release when merged.

# Conflicts:
#	package-lock.json
#	package.json
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b13fd0f) 100.00% compared to head (efbf2a3) 100.00%.
Report is 20 commits behind head on main.

❗ Current head efbf2a3 differs from pull request most recent head 55bfef9. Consider uploading reports for the commit 55bfef9 to get more accurate results

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.
📢 Have feedback on the report? Share it here.

@solaris007 solaris007 added the enhancement New feature or request label Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants