feat(uaa-logging): Log UAA Parity data #3629
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/api/Feed.js
Outdated
|
|
||
| const parsedFeedItems = getParsedFileActivitiesResponse(response); | ||
| const parsedDataParity = { v2: sortedFeedItems, uaa: parsedFeedItems }; | ||
| handleFeedItems(parsedFeedItems, { responseParity, parsedDataParity }); |
There was a problem hiding this comment.
So we're not going to handle the feed items until all of the promises resolve? Will this double the time for the results to be shown to the user. Should the parity check not be an asynchronous process that will log an error message if the results from UAA are different from the results coming from the individual api calls?
There was a problem hiding this comment.
That makes sense, I updated the PR so that the UI is not blocked by both responses. Now the v2 calls will be async and will not be blocking and will be used only for logging.
JChan106
force-pushed
the
uaa-logging
branch
from
19dc4d0 to
f256865
Compare
src/elements/common/logger/Logger.js
Outdated
| ...data, | ||
| type: METRIC_TYPE_UAA_PARITY_METRIC, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
❓ do we want this return? Won't this prevent METRIC_TYPE_PREVIEW logging below?
There was a problem hiding this comment.
shouldn't be an issue since the METRIC_TYPE_PREVIEW would be a separate event. Ill change this to an if/else to make it more readable.
There was a problem hiding this comment.
an approach that comes to mind is to leave Feed.js untouched and use the successCallback/errorCallback passed into feedItems to handle the logging. ActivitySidebar would need to call api.getFeedAPI twice with different values for shouldUseUAA. that way all of the logging logic could live together in one location instead of split between the api and the component
but approving because this looks to be short term until parity is verified and feature is GA
| @@ -42,6 +42,7 @@ import { | |||
| ORIGIN_ACTIVITY_SIDEBAR, | |||
| SIDEBAR_VIEW_ACTIVITY, | |||
| TASK_COMPLETION_RULE_ALL, | |||
| METRIC_TYPE_UAA_PARITY_METRIC, | |||
There was a problem hiding this comment.
nit: this list is alphasorted
| @@ -550,6 +551,7 @@ class Feed extends Base { | |||
| shouldShowVersions?: boolean, | |||
| shouldUseUAA?: boolean, | |||
| } = {}, | |||
| logAPIParity?: Function, | |||
There was a problem hiding this comment.
nit: function header definition
| : Promise.resolve(); | ||
| const annotationsPromise = shouldShowAnnotations | ||
| ? this.fetchAnnotations(permissions, shouldShowReplies) | ||
| : Promise.resolve(); | ||
| const commentsPromise = () => { |
There was a problem hiding this comment.
nit: commentsPromise could be updated to match the ternary pattern of the others?
| const fetchV2FeedItems = async promises => { | ||
| return Promise.all(promises).then( | ||
| ([versions: ?FileVersions, currentVersion: ?BoxItemVersion, ...feedItems]) => { | ||
| const versionsWithCurrent = currentVersion | ||
| ? this.versionsAPI.addCurrentVersion(currentVersion, versions, this.file) | ||
| : undefined; | ||
| return sortFeedItems(versionsWithCurrent, ...feedItems); | ||
| }, | ||
| ); | ||
| }; | ||
|
|
||
| const compareV2AndUaaFeedItems = async (uaaFeedItems, uaaResponse) => { |
There was a problem hiding this comment.
i don't think you need async in either function since await is not being used
| const compareV2AndUaaFeedItems = async (uaaFeedItems, uaaResponse) => { | ||
| fetchV2FeedItems(v2Promises).then(v2FeedItems => { | ||
| const transformedV2FeedItems = collapseFeedState(v2FeedItems); | ||
| const transformedUAAFeedItems = collapseFeedState(uaaFeedItems); |
There was a problem hiding this comment.
nit: inconsistency of uaa between this variable transformedUAAFeedItems and the function name compareV2AndUaaFeedItems
| @@ -89,6 +89,7 @@ import type { | |||
| Tasks, | |||
| ThreadedComments as ThreadedCommentsType, | |||
| } from '../common/types/feed'; | |||
| import { collapseFeedState } from '../elements/content-sidebar/activity-feed/activity-feed/activityFeedUtils'; | |||
There was a problem hiding this comment.
i think it's odd for an api file to be importing from a component utility file
No description provided.