Add manual tracing to Identity #5283

xirzec · 2019-09-27T22:30:34Z

This PR:

Adds tracing to the Identity client with manual Span propagation
Creates a new TestTracer for use in unit tests
Adds some unit tests to Identity that use TestTracer
Removes an unnecessary interface from core-tracing
Adds SpanOptions to RequestPrepareOptions in core-http

This PR cleans up a previous effort by Sarangan (pull #5019).

Love to get feedback on the feel of the tests, if there is anything folks would like to see done differently.

Update master

Merge upstream

chradek

Overall looks good! "Hide whitespace changes" helped ;)

sdk/identity/identity/src/client/identityClient.ts

chradek · 2019-09-27T23:14:10Z

sdk/identity/identity/src/client/identityClient.ts

+        span.setStatus({
+          code: CanonicalCode.UNAUTHENTICATED,
+          message: err.message
+        })


Nit: missing semi-colon.

Good eyes! Definitely need to get lint working...

chradek · 2019-09-27T23:40:08Z

sdk/identity/identity/src/credentials/deviceCodeCredential.ts

-    return tokenResponse;
+      return tokenResponse;
+    } catch (err) {
+      const code = err instanceof AuthenticationError ? CanonicalCode.UNAUTHENTICATED : CanonicalCode.UNKNOWN;


Would it be safer to check the err.name for AuthenticationError?

So it's true that errors from another context (e.g. iframe) will fail instanceof checks, but the downside of using name is that if you want inheritance in your Error objects then you can't handle classes of errors together.

E.g. you can imagine doing something like

class ValidationError extends Error {} class RangeError extends ValidationError {} try { // something } catch (e) { if (e instanceof ValidationError) { // this works for RangeError! } }

I'm flexible on if we want to move to a name-based pattern (or some other approach), but since Identity was already relying on instanceof AuthenticationError in other code, I think keeping it consistent is fine for now.

/cc @bterlson if he has any design guidance for us.

I talked to Brian. This usage of instanceof probably is safe, but he was generally in favor of moving to checking name since we don't have plans for hierarchical errors anytime soon. I'll look at switching this over.

Gotcha. I think we had had some issue when using custom errors in core-amqp that made us switch to doing the name check but I’ll have to double-check to confirm. Didn’t realize it was already being done in the rest of the SDK and generally agree to favor consistency.

Moved this over to using name checks, please take another look

chradek · 2019-09-30T19:32:35Z

sdk/identity/identity/src/credentials/managedIdentityCredential.ts

+          (err.code === RestError.REQUEST_SEND_ERROR || err.code === RestError.REQUEST_ABORTED_ERROR)
+        ) {
+          // Either request failed or IMDS endpoint isn't available
+          return false;


Should the status of the span be a failure in this case? Unauthenticated?

I think UNAVAILABLE makes more sense here, since we're checking the availability of the endpoint. Good spotting that catch wasn't rethrowing here.

daviwil

Looks great for the most part! The testing approach looks solid. A few other comments below

daviwil · 2019-09-30T20:35:17Z

sdk/identity/identity/src/credentials/chainedTokenCredential.ts

      } catch (err) {
+        span.setStatus({


The purpose of this loop is to try each credential in the chain until one is able to authenticate successfully. If there are 3 credentials in the chain and the first two throw an exception but the last succeeds, shouldn't the span be considered successful? Maybe we should only trace in line 44/53 below where the AggregateAuthenticationError gets thrown and use the message from that exception instead.

The span is specific to the work it wraps. I think we do actually want to see that some spans failed, as it's the trace that will ultimately succeed (if there's a top-level span that wraps the chain, that could indicate success or failure as well).

For example, it can be useful for the customer to see that the 1st credential always fails, or only the 1st and 3rd pass. Then they could create their own chain if they wanted. Or maybe they think they are using one credential but it turns out that one is failing and they always fall back to another.

Oh I see, the span is started outside the loop, not inside. (Missed that context when just reading the comment).

daviwil · 2019-09-30T20:36:06Z

sdk/identity/identity/src/credentials/clientCertificateCredential.ts

+      return (tokenResponse && tokenResponse.accessToken) || null;
+    } catch (err) {
+      span.setStatus({
+        code: CanonicalCode.UNKNOWN,


When should we use UNKNOWN versus UNAUTHENTICATED? Seems like a distinction might need to be made on the error type as done elsewhere in this PR.

UNKNOWN is the generic failure; I agree with adding the error type checking here.

daviwil · 2019-09-30T20:36:56Z

sdk/identity/identity/src/credentials/clientSecretCredential.ts

+      return (tokenResponse && tokenResponse.accessToken) || null;
+    } catch (err) {
+      span.setStatus({
+        code: CanonicalCode.UNKNOWN,


Maybe UNAUTHENTICATED?

sdk/identity/identity/src/credentials/environmentCredential.ts

daviwil · 2019-09-30T20:41:33Z

sdk/identity/identity/src/credentials/managedIdentityCredential.ts

+      return (tokenResponse && tokenResponse.accessToken) || null;
+    } catch (err) {
+      span.setStatus({
+        code: CanonicalCode.UNKNOWN,


Maybe UNAUTHENTICATED?

daviwil · 2019-09-30T20:42:21Z

sdk/identity/identity/src/credentials/usernamePasswordCredential.ts

+      return (tokenResponse && tokenResponse.accessToken) || null;
+    } catch (err) {
+      span.setStatus({
+        code: CanonicalCode.UNKNOWN,


Maybe UNAUTHENTICATED?

daviwil

Looks good, thanks a lot!

xirzec added 15 commits September 23, 2019 12:38

Merge pull request #1 from Azure/master

d30bf68

Update master

Merge pull request #2 from Azure/master

8960a89

Merge upstream

Merge pull request #3 from Azure/master

4dd8b02

Merge upstream

Add SpanOptions to RequestPrepareOptions

51facec

Add tracing to identityClient

4b5edd6

Remove unnecessary interface

db59a97

Fixup runtime dependency on an OpenCensus enum

14e9186

Add identity tracing to public methods

802c540

Directory rename

f896a37

Create Test Tracer and Span implementations

21259a3

Add test helpers to TestTracer

a67d1b8

Add ClientCertificateCredential test

49d008b

fixup malformed tsconfig.json

e01acce

Add DeviceCodeCredential test

12cbd2f

Add EnvironmentCredential test

bad5f0b

xirzec added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Sep 27, 2019

xirzec requested review from bterlson, daviwil, sophiajt, maggiepint, ramya-rao-a and chradek September 27, 2019 22:30

xirzec requested a review from sadasant as a code owner September 27, 2019 22:30

xirzec self-assigned this Sep 27, 2019

xirzec mentioned this pull request Sep 27, 2019

Add Tracing to Identity #5019

Closed

chradek reviewed Sep 27, 2019

View reviewed changes

chradek mentioned this pull request Sep 27, 2019

[Event Hubs] add tracing support when sending events #5207

Merged

xirzec added 2 commits September 27, 2019 17:03

Add missing semicolon

c59c337

Address PR feedback

b47a1bb

xirzec requested a review from chradek September 30, 2019 19:25

chradek reviewed Sep 30, 2019

View reviewed changes

Address PR feedback

500248f

xirzec requested a review from chradek September 30, 2019 19:53

chradek approved these changes Sep 30, 2019

View reviewed changes

daviwil reviewed Sep 30, 2019

View reviewed changes

Address PR feedback

34cbd94

xirzec requested a review from daviwil September 30, 2019 21:01

daviwil approved these changes Sep 30, 2019

View reviewed changes

xirzec merged commit fc0887f into Azure:master Sep 30, 2019

xirzec deleted the identityTracing branch September 30, 2019 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add manual tracing to Identity #5283

Add manual tracing to Identity #5283

xirzec commented Sep 27, 2019

chradek left a comment

chradek Sep 27, 2019

xirzec Sep 27, 2019

chradek Sep 27, 2019

xirzec Sep 28, 2019

xirzec Sep 28, 2019

chradek Sep 28, 2019

xirzec Sep 30, 2019

chradek Sep 30, 2019

xirzec Sep 30, 2019

daviwil left a comment

daviwil Sep 30, 2019

chradek Sep 30, 2019

chradek Sep 30, 2019

daviwil Sep 30, 2019

xirzec Sep 30, 2019

daviwil Sep 30, 2019

daviwil Sep 30, 2019

daviwil Sep 30, 2019

daviwil left a comment

Add manual tracing to Identity #5283

Add manual tracing to Identity #5283

Conversation

xirzec commented Sep 27, 2019

chradek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviwil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviwil left a comment

Choose a reason for hiding this comment