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

Update Emulator to Use New Cognitive Service API #1878

Merged
merged 1 commit into from Oct 8, 2019

Conversation

@bwamie
Copy link

bwamie commented Sep 22, 2019

Update Emulator to Use New Cognitive Service API
Add token refresh 5 minutes before it expires.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 22, 2019

Coverage Status

Coverage increased (+0.02%) to 66.959% when pulling e2ccb9a on edwinb/speech-for-paid-api-changes into 98e7b21 on master.

Copy link
Contributor

compulim left a comment

Add tests.

packages/app/client/src/state/sagas/chatSagas.ts Outdated Show resolved Hide resolved
packages/emulator/core/src/facility/botEndpoint.ts Outdated Show resolved Hide resolved
packages/emulator/core/src/facility/botEndpoint.ts Outdated Show resolved Hide resolved
packages/sdk/shared/src/types/index.ts Outdated Show resolved Hide resolved
packages/sdk/shared/src/types/speechRegionToken.ts Outdated Show resolved Hide resolved
if (!this.msaAppId || !this.msaPassword) {
throw new Error('bot must have Microsoft App ID and password');
}
private isTokenExpired(): boolean {

This comment has been minimized.

Copy link
@compulim

compulim Sep 23, 2019

Contributor

Don't duplicate code.

This comment has been minimized.

Copy link
@bwamie

bwamie Sep 24, 2019

Author

With which one? Its not duplacted. If you mean with isTokenExpiringSoon then it is not. They are different.

This comment has been minimized.

Copy link
@compulim

compulim Sep 26, 2019

Contributor

I mean the isTokenExpired and isTokenExpiringSoon is too similar.

It may make sense to return the expiry, or building a function named willTokenExpireWithin(durationInMs: number = 0).


const query = new URLSearchParams({ goodForInMinutes: duration } as any);
const res = await this.fetchWithAuth(new URL(`?${query.toString()}`, speechEndpoint.tokenEndpoint).toString());
private renewTokenBeforeExpiry() {

This comment has been minimized.

Copy link
@compulim

compulim Sep 23, 2019

Contributor

Don't use then, use async/await.

@@ -0,0 +1,38 @@
//

This comment has been minimized.

Copy link
@compulim

compulim Sep 23, 2019

Contributor

Why there is a dupe file in /sdk/?

This comment has been minimized.

Copy link
@bwamie

bwamie Sep 23, 2019

Author

Same as above

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Sep 24, 2019

Contributor

We should just choose one -- I vote for sdk -- and delete the other one. There should be minimal code churn because the type SpeechTokenInfo is only used in one place

packages/app/shared/src/types/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

tonyanziano left a comment

After you finish resolving PR feedback, please run the command npm run lint:fix from the root of the project. This should fix issues like import sorting, however it might not alphabetize them.

@@ -0,0 +1,38 @@
//

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Sep 24, 2019

Contributor

We should just choose one -- I vote for sdk -- and delete the other one. There should be minimal code churn because the type SpeechTokenInfo is only used in one place

}

if (!this.msaAppId || !this.msaPassword) {
throw new Error('bot must have Microsoft App ID and password');

This comment has been minimized.

Copy link
@compulim

compulim Sep 26, 2019

Contributor

I mean it looks a bit late to throw here.

Copy link
Contributor

tonyanziano left a comment

Pulled it down and tested it. Looks good to me, but you should address the comments William left. 👍

packages/app/client/src/state/sagas/chatSagas.ts Outdated Show resolved Hide resolved
packages/emulator/core/src/facility/botEndpoint.ts Outdated Show resolved Hide resolved
}

if (this.speechRegionToken && !refresh && !this.willTokenExpireWithin(0)) {
if (this.willTokenExpireWithin(this.speechRegionToken.tokenLife * 0.5)) {

This comment has been minimized.

Copy link
@compulim

compulim Sep 30, 2019

Contributor
Suggested change
if (this.willTokenExpireWithin(this.speechRegionToken.tokenLife * 0.5)) {
if (this.willTokenExpireWithin(this.speechRegionToken.tokenLife / 2)) {
}
private willTokenExpireWithin(millisecondsToExpire: number): boolean {
const currentDateTime = new Date();
const timeFromNowMilliseconds = currentDateTime.getTime() + millisecondsToExpire;

This comment has been minimized.

Copy link
@compulim

compulim Sep 30, 2019

Contributor
Suggested change
const timeFromNowMilliseconds = currentDateTime.getTime() + millisecondsToExpire;
const timeFromNowMilliseconds = Date.now() + millisecondsToExpire;
packages/emulator/core/src/authEndpoints.ts Outdated Show resolved Hide resolved
packages/app/client/src/state/sagas/chatSagas.ts Outdated Show resolved Hide resolved
packages/emulator/core/src/facility/botEndpoint.ts Outdated Show resolved Hide resolved
packages/emulator/core/src/facility/botEndpoint.ts Outdated Show resolved Hide resolved
packages/emulator/core/src/facility/botEndpoint.ts Outdated Show resolved Hide resolved
Copy link
Contributor

compulim left a comment

In addition for comments below, there are couple of things that are still unanswered in previous reviews.


if (speechAuthenticationToken && speechAuthenticationToken.access_Token && speechAuthenticationToken.region) {
const factory = yield call(createCognitiveServicesSpeechServicesPonyfillFactory, {
authorizationToken: () => Promise.resolve(speechAuthenticationToken.access_Token),

This comment has been minimized.

Copy link
@compulim

compulim Oct 2, 2019

Contributor

This should a Promise function. When called, will fetch the token from the server. And the token need to be cached for its half-life.

@corinagum corinagum assigned tonyanziano and unassigned corinagum Oct 4, 2019
@tonyanziano tonyanziano force-pushed the edwinb/speech-for-paid-api-changes branch from 9e4d7f5 to 505fd53 Oct 7, 2019
@tonyanziano tonyanziano force-pushed the edwinb/speech-for-paid-api-changes branch from 505fd53 to da0ef01 Oct 7, 2019
Copy link
Contributor

compulim left a comment

Looks good to me.

If the speech token call is taking too much time, and the user keep clicking on the microphone button. It may end up having multiple ongoing "get token call". I am okay with this for now.

Love your tests. AFAIR, people said test code is statistically 3 times the size of production code.

Let's wait for Chris to test it out.

packages/emulator/core/src/authEndpoints.ts Outdated Show resolved Hide resolved
@cwhitten cwhitten dismissed stale reviews from compulim and tonyanziano Oct 8, 2019

stale

@cwhitten

This comment has been minimized.

Copy link
Member

cwhitten commented Oct 8, 2019

Nice work @edwin4microsoft & @tonyanziano. Let's resolve conflicts and get it in.

@tonyanziano tonyanziano force-pushed the edwinb/speech-for-paid-api-changes branch from da0ef01 to 06b0443 Oct 8, 2019
@tonyanziano tonyanziano force-pushed the edwinb/speech-for-paid-api-changes branch from 06b0443 to e2ccb9a Oct 8, 2019
@tonyanziano tonyanziano merged commit 90384b8 into master Oct 8, 2019
2 checks passed
2 checks passed
Emulator-CI-PR #82125 succeeded
Details
license/cla All CLA requirements met.
Details
@tonyanziano tonyanziano deleted the edwinb/speech-for-paid-api-changes branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.