Add Tracing to Identity #5019

sarangan12 · 2019-09-05T22:56:54Z

I have made changes to core-http, core-auth & identity. I have not updated any package versions. Will discuss with Ramya and update package versions on Monday

bterlson · 2019-09-06T23:54:42Z

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

@@ -86,6 +89,9 @@ export class IdentityClient extends ServiceClient {
      return null;
    }

+    const span = createSpan("IdentityClient-refreshAccessToken", getSpanOptions(options));


Where is this span name convention coming from? Is this aligning with other languages?

bterlson · 2019-09-07T00:37:40Z

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

    } catch (err) {
-      if (err instanceof AuthenticationError && err.errorResponse.error === "interaction_required") {
+      span.end();


I'd put span.end() in a finally block rather than in both the try and catch legs.

That would work if the catch block was not throwing the error again like it does here

bterlson · 2019-09-07T00:50:27Z

sdk/identity/identity/src/util/tracingUtils.ts

@@ -0,0 +1,37 @@
+// Copyright (c) Microsoft Corporation.


Should these helpers move to some core library?

The getSpanOptions is not required. A call to this can be replaced with options && options.spanOptions

The createSpan can indeed move to core-tracing, but I would keep work out of this PR.

ramya-rao-a · 2019-09-08T00:02:47Z

sdk/identity/identity/src/util/tracingUtils.ts

+  const tracer = TracerProxy.getTracer();
+  const span = tracer.startSpan(name, spanOptions);
+  if (tracer.pluginType !== SupportedPlugins.NOOP && (spanOptions && spanOptions.parent)) {
+    spanOptions = { ...spanOptions, parent: span };


This will only update the local variable spanOptions and not the actual spanOptions on the outer options of type GetTokenOptions which is what we want right?

Updating the local variable serves no purpose.

I would suggest the below

Update createSpan to take options of type GetTokenOptions as the second parameter

Then the above line would become options.spanOptions = { ...options.spanOptions, parent: span };

On second thoughts, I realize that updating the parent on the spanOptions might not be a good idea to do inside createSpan.

There are cases in this PR where we pass the same options (of type GetTokenOptions) to multiple methods which internally create more spans. Having the options updated inside createSpan will mess these parallel calls that are happening.

Essentially, we might have to clone the options each time it gets passed around.

ramya-rao-a · 2019-09-08T00:07:15Z

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

+      abortSignal: options && options.abortSignal,
+      spanOptions: {
+        parent: span
+      }


Here, we are assuming that parent is the only thing we want to care about in span options.
The customer might be using a tracer that would take in more items in the options bag. The customer might have provided that, but we end up ignoring it here.

ramya-rao-a

@sarangan12 I have taken a look at most files and my main feedback is around updating createSpan to take the GetTokenOptions to avoid setting the parent outside of createSpan.

Please take a look if that is doable. Also, I have the PR #5055 out to format the files in identity. Hopefully we can have that merged first and then update your branch from master. This way, we can remove all the formatting noise out of this PR

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

…azure-sdk-for-js into AddTracingToIdentity

mikeharder · 2019-09-09T17:48:01Z

common/config/rush/pnpm-lock.yaml

@@ -8,7 +8,9 @@ dependencies:
  '@azure/ms-rest-js': 2.0.4
  '@azure/ms-rest-nodeauth': 0.9.3
  '@azure/storage-blob': 12.0.0-preview.2
-  '@microsoft/api-extractor': 7.3.8
+  '@microsoft/api-extractor': 7.3.11


Was this file updated by running rush update or rush update --full? It appears to be the latter, since unrelated dependencies are being updated. Can you please revert this file and run rush update to just add/update the dependencies modified in this PR?

mikeharder · 2019-09-09T17:50:03Z

sdk/identity/identity/package.json

@@ -80,6 +80,9 @@
  },
  "devDependencies": {
    "@azure/abort-controller": "1.0.0-preview.2",
+    "@opencensus/core": "~0.0.17",


Most dependencies should use ^. Is there a reason these new dependencies use ~ instead?

xirzec · 2019-09-18T18:15:31Z

sdk/identity/identity/src/util/tracingUtils.ts

+ * @param options options to be modified
+ */
+
+export function modifySpanOptions(span: Span, options?: GetTokenOptions): GetTokenOptions {


Really this isn't modifying span options so much as it is creating a new span options for the child span manufactured in createSpan.

Since this method always gets called after createSpan, would it make more sense to have createSpan return an object of { span, options } so you can destructure the result and not need to call two methods each time?

Yes, please. I was about to make a similar suggestion in another place in this PR. I think it's inconvenient to always have to call two methods, and also possibly easy to forget.

xirzec · 2019-09-18T18:19:39Z

sdk/identity/identity/test/node/clientCertificateCredential.spec.ts

+        1,
+        `Number of children ${rootSpan.numberOfChildren} is not equal to 1.`
+      );
+      const numberOfDescendants = rootSpan.allDescendants().length;


Should we add some kind of validator for the rootSpan that can understand more than just the child count / descendant count?

In chromium we'd use golden files for this and write out all the events with things like timestamps sanitized, but maybe in this case we could just have a helper function that takes a tree-like structure of the expected events?

xirzec · 2019-09-18T18:23:15Z

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

@@ -32,6 +33,10 @@ export class ChainedTokenCredential implements TokenCredential {
    let token = null;
    const errors = [];

+    const span = createSpan("ChainedTokenCredential-getToken", options);
+    options = modifySpanOptions(span, options);


nit: personally, I think it's bad form to mutate function parameters. I prefer to treat them as immutable and create new local versions when necessary.

xirzec · 2019-09-27T23:12:44Z

Superceded this PR with #5283

Add Tracing to Identity

367bead

sarangan12 requested review from daviwil, sophiajt and ramya-rao-a September 5, 2019 22:56

ramya-rao-a requested review from bterlson and chradek September 6, 2019 06:29

sarangan12 added 2 commits September 6, 2019 15:58

Code changes to pass spans correctly

54a6d4d

Changes due to rush update

dd40857

sarangan12 requested review from KarishmaGhiya, mikeharder and ShivangiReja as code owners September 6, 2019 22:59

sarangan12 changed the title ~~Add Tracing to Identity - WIP - OK for Initial Review~~ Add Tracing to Identity Sep 6, 2019

sarangan12 added 2 commits September 6, 2019 16:07

Merge remote-tracking branch 'origin/master' into AddTracingToIdentity

4a67ebd

Code due to rush update full

f2f61b2

bterlson requested changes Sep 7, 2019

View reviewed changes

ramya-rao-a reviewed Sep 8, 2019

View reviewed changes

ramya-rao-a mentioned this pull request Sep 8, 2019

[Identity] Format all files #5055

Merged

ramya-rao-a suggested changes Sep 8, 2019

View reviewed changes

ramya-rao-a removed request for daviwil, KarishmaGhiya, mikeharder and ShivangiReja September 8, 2019 00:32

ramya-rao-a mentioned this pull request Sep 8, 2019

Update GetTokenOptions to support tracing #5057

Merged

ramya-rao-a added 2 commits September 8, 2019 14:08

Merge remote-tracking branch 'origin/master' into pr/sarangan12/5019

c1504ad

Updating lock file

76dcee9

sarangan12 requested a review from daviwil as a code owner September 8, 2019 21:18

ramya-rao-a reviewed Sep 9, 2019

View reviewed changes

sdk/identity/identity/src/client/identityClient.ts Show resolved Hide resolved

sarangan12 added 2 commits September 9, 2019 08:30

Minor refactor

18730f6

Merge branch 'AddTracingToIdentity' of https://github.com/sarangan12/…

e9e785e

…azure-sdk-for-js into AddTracingToIdentity

sarangan12 added 3 commits September 9, 2019 08:38

Add span to refreshAccessToken createwebresource

5479b64

Merge remote-tracking branch 'origin/master' into AddTracingToIdentity

180aef4

Code Refactor

5c841da

mikeharder requested changes Sep 9, 2019

View reviewed changes

xirzec reviewed Sep 18, 2019

View reviewed changes

xirzec mentioned this pull request Sep 27, 2019

Add manual tracing to Identity #5283

Merged

5 tasks

xirzec closed this Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tracing to Identity #5019

Add Tracing to Identity #5019

sarangan12 commented Sep 5, 2019 •

edited

Loading

bterlson Sep 6, 2019

bterlson Sep 7, 2019

ramya-rao-a Sep 9, 2019

bterlson Sep 7, 2019

ramya-rao-a Sep 7, 2019

ramya-rao-a Sep 8, 2019 •

edited

Loading

ramya-rao-a Sep 9, 2019

ramya-rao-a Sep 8, 2019 •

edited

Loading

ramya-rao-a left a comment

mikeharder Sep 9, 2019

mikeharder Sep 9, 2019

xirzec Sep 18, 2019

daviwil Sep 24, 2019

xirzec Sep 18, 2019

xirzec Sep 18, 2019

xirzec commented Sep 27, 2019

Add Tracing to Identity #5019

Add Tracing to Identity #5019

Conversation

sarangan12 commented Sep 5, 2019 • edited Loading

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

ramya-rao-a Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

ramya-rao-a 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

xirzec commented Sep 27, 2019

sarangan12 commented Sep 5, 2019 •

edited

Loading

ramya-rao-a Sep 8, 2019 •

edited

Loading

ramya-rao-a Sep 8, 2019 •

edited

Loading