Skip to content

enrich disintigration #478

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 13, 2021
Merged

enrich disintigration #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ module.exports = {
TC_API: process.env.TC_API || 'https://api.topcoder-dev.com/v5',
// the organization id
ORG_ID: process.env.ORG_ID || '36ed815b-3da1-49f1-a043-aaed0a4e81ad',
// the referenced skill provider id
TOPCODER_SKILL_PROVIDER_ID: process.env.TOPCODER_SKILL_PROVIDER_ID || '9cc0795a-6e12-4c84-9744-15858dba1861',
// the referenced taxonomy id
TOPCODER_TAXONOMY_ID: process.env.TOPCODER_TAXONOMY_ID || '7637ae1a-3b7c-44eb-a5ed-10ea02f1885d',

TOPCODER_USERS_API: process.env.TOPCODER_USERS_API || 'https://api.topcoder-dev.com/v3/users',
// the api to find topcoder members
Expand Down
48 changes: 27 additions & 21 deletions docs/swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3157,7 +3157,7 @@ paths:
schema:
type: array
items:
$ref: "#/components/schemas/UbahnSkill"
$ref: "#/components/schemas/SkillInSkillsAPI"
headers:
X-Next-Page:
schema:
Expand Down Expand Up @@ -5319,17 +5319,25 @@ components:
type: string
example: "React"
description: The skill name.
UbahnSkill:
type: object
SkillInSkillsAPI:
Copy link

Choose a reason for hiding this comment

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

@Sande3p do we need to specify Skills API swagger here? I mean now as api is extracted, we don't need its swagger here as well, right? or Am I missing something here?

required:
- "id"
- "name"
- "taxonomyId"
- "taxonomyName"
- "metadata"
properties:
id:
type: "string"
format: "UUID"
description: "The skill id"
skillProviderId:
taxonomyId:
type: "string"
format: "UUID"
description: "The referenced skill provider id"
description: "The referenced taxonomy id"
taxonomyName:
type: "string"
description: "The referenced taxonomy name"
name:
type: "string"
description: "The name of the skill"
Expand All @@ -5339,22 +5347,20 @@ components:
uri:
type: "string"
description: "The uri for the skill"
created:
type: "string"
format: "date-time"
description: "When the entity was created."
updated:
type: "string"
format: "date-time"
description: "When the entity was updated."
createdBy:
type: "string"
format: "UUID"
description: "Creator of the entity."
updatedBy:
type: "string"
format: "UUID"
description: "User that last updated the entity."
metadata:
type: "object"
description: "The metadata for the skill"
properties:
updated:
type: "string"
format: "date-time"
description: "The last updated timestamp of the skill"
challengeProminence:
type: "string"
description: "The challenge prominence ranging from [0, 1]"
memberProminence:
type: "string"
description: "The member prominence ranging from [0, 1]"
JobForTeam:
properties:
id:
Expand Down
29 changes: 19 additions & 10 deletions src/common/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,15 @@ async function listUsersByExternalId (externalId) {
context: 'listUserByExternalId',
message: `response body: ${JSON.stringify(res.body)}`
})
return res.body

const users = res.body
// populate skill data for each user skill
await Promise.all(users.map(user => Promise.all(user.skills.map(async userSkill => {
const skill = await getSkillById(userSkill.skillId)
Copy link

Choose a reason for hiding this comment

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

@Sande3p I think we should make a call to getAllTopcoderSkills first and then lookup for the user skills there. It would prevent multiple API calls to the skill api. If one user has 50 skills and the ubahn user query returned 100 users, it would make 5000 api calls, which is not at all feasible.
I remember seeing the cache of getAllTopcoderSkills somewhere in our code base, but not able to locate it right now. If we can have that cache loaded on app loading, it would resolve more problems. @maxceem need your thoughts here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vikasrohit the previous code was already not optimized and it loaded skills individually. The only "caching" place which I know is where we are saving matching of u-bahn skills with emsi skills https://github.com/topcoder-platform/taas-apis/blob/feature/notifications-api/scripts/emsi-mapping/index.js#L50, but I doubt that could be used as a cache here.

In a nutshell, if you just would like to update TaaS API as per new Skills API, then this PR seems to be not needed, and other PR is enough https://github.com/topcoder-platform/taas-apis/pull/460/files.
But if you would like to additionally take care of performance, then suggested way with getAllTopcoderSkills might be a great idea to try. We have some pages on DEV which load a bit slow like this one https://platform.topcoder-dev.com/taas/myteams/16786, though I'm not 100% sure if it's caused by skills or no.

Copy link

Choose a reason for hiding this comment

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

@maxceem in the previous code it didn't need to be optimized because with enrich feature users objects were already having the full data they need i.e. they already have skills names in every userSkill object. So, I guess there was not a performance issue there but in the new code now we are making separate API calls to fetch the skills data.

Copy link
Contributor

@maxceem maxceem Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@vikasrohit Got it, I've missed that in this place we didn't have loading individual skills before. Then I would suggest keep the minimal changes in this PR:

  • still load skills individually as it's done in all other places
  • take into account you comments: most of the changes are not needed as we can _.pick(skillObj.skill, ['id', 'name']) inside the service
  • if we see speed degradation in skills loading for users, then we should implement caching for all places at once by caching getAllTopcoderSkills and using it in all such places instead of individual calls

Copy link

@vikasrohit vikasrohit Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@maxceem Yes, I too want the PR to be as lean as possible. However, I don't think we need to wait for testing it with respect to performance, because I don't any other place in the API where we are making similar number of API calls for fetching the individual skills. So, it has very high probability of performance impact. Isn't it? Do you see any other similar level API calls in the existing code?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes @vikasrohit there are such places, and they even are updated in this PR. If you check other places which are updated in this PR, there we are already requesting multiple skills by ids:

Code was refactored but the call getSkillById was already there. In these places we populate Skill names for Jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a picture from UI point of view:

image

So it may happen, that when we start loading skills individually for users too. This page might become too slow. But if we optimize such calls for users, it make sense to optimize such calls in existent places too all together.

See page example https://platform.topcoder-dev.com/taas/myteams/16786

Copy link

Choose a reason for hiding this comment

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

Great details @maxceem. That would make it clear from QA perspective as well. @lakshmiathreya @SathyaJayabal fyi.

userSkill.skill = skill
}))))

return users
}

/**
Expand Down Expand Up @@ -1082,9 +1090,10 @@ async function getUserById (userId, enrich) {
const user = _.pick(res.body, ['id', 'handle', 'firstName', 'lastName'])

if (enrich) {
user.skills = (res.body.skills || []).map((skillObj) =>
_.pick(skillObj.skill, ['id', 'name'])
)
user.skills = await Promise.all((res.body.skills || []).map(async (userSkill) => {
const skill = await getSkillById(userSkill.skillId)
Copy link

Choose a reason for hiding this comment

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

Same as with above comment for listUsersByExternalId. We should not make separate call for each skill.

return _.pick(skill, ['id', 'name'])
}))
const attributes = _.get(res, 'body.attributes', [])
user.attributes = _.map(attributes, (attr) =>
_.pick(attr, ['id', 'value', 'attribute.id', 'attribute.name'])
Expand Down Expand Up @@ -1201,18 +1210,18 @@ async function getProjectById (currentUser, id) {

/**
* Function to search skills from v5/skills
* - only returns skills from Topcoder Skills Provider defined by `TOPCODER_SKILL_PROVIDER_ID`
* - only returns skills from Topcoder Skills API defined by `TOPCODER_TAXONOMY_ID`
*
* @param {Object} criteria the search criteria
* @returns the request result
*/
async function getTopcoderSkills (criteria) {
const token = await getM2MUbahnToken()
const token = await getM2MToken()
try {
const res = await request
.get(`${config.TC_API}/skills`)
.query({
skillProviderId: config.TOPCODER_SKILL_PROVIDER_ID,
taxonomyId: config.TOPCODER_TAXONOMY_ID,
...criteria
})
.set('Authorization', `Bearer ${token}`)
Expand All @@ -1238,7 +1247,7 @@ async function getTopcoderSkills (criteria) {

/**
* Function to search and retrive all skills from v5/skills
* - only returns skills from Topcoder Skills Provider defined by `TOPCODER_SKILL_PROVIDER_ID`
* - only returns skills from Topcoder Skills API defined by `TOPCODER_TAXONOMY_ID`
*
* @param {Object} criteria the search criteria
* @returns the request result
Expand All @@ -1260,7 +1269,7 @@ async function getAllTopcoderSkills (criteria) {
* @returns the request result
*/
async function getSkillById (skillId) {
const token = await getM2MUbahnToken()
const token = await getM2MToken()
const res = await request
.get(`${config.TC_API}/skills/${skillId}`)
.set('Authorization', `Bearer ${token}`)
Expand All @@ -1270,7 +1279,7 @@ async function getSkillById (skillId) {
context: 'getSkillById',
message: `response body: ${JSON.stringify(res.body)}`
})
return _.pick(res.body, ['id', 'name'])
Copy link

Choose a reason for hiding this comment

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

Just curious why we need to remove selective picking fields? Do we need any other field where this method is being consumer?

return res.body
}

/**
Expand Down
16 changes: 8 additions & 8 deletions src/services/TeamService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,16 +337,13 @@ async function getTeam (currentUser, id) {
const teamDetail = result[0]

// add job skills for result
let jobSkills = []
Copy link

Choose a reason for hiding this comment

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

@Sande3p I don't understand the need of changes in this file and as previously mentioned changes for getSkillById (removing selective picking of fields in the helper method itself rather doing it every caller) also seems to be non required to me.

if (teamDetail && teamDetail.jobs) {
for (const job of teamDetail.jobs) {
if (job.skills) {
const usersPromises = []
_.map(job.skills, (skillId) => {
usersPromises.push(helper.getSkillById(skillId))
})
jobSkills = await Promise.all(usersPromises)
job.skills = jobSkills
job.skills = await Promise.all(job.skills.map(async (skillId) => {
const skill = await helper.getSkillById(skillId)
return _.pick(skill, ['id', 'name'])
}))
}
}
}
Expand Down Expand Up @@ -385,7 +382,10 @@ async function getTeamJob (currentUser, id, jobId) {

if (job.skills) {
result.skills = await Promise.all(
_.map(job.skills, (skillId) => helper.getSkillById(skillId))
_.map(job.skills, async (skillId) => {
const skill = await helper.getSkillById(skillId)
return _.pick(skill, ['id', 'name'])
})
)
}

Expand Down