Add tracing to keyvault-keys library by ramya-rao-a · Pull Request #4654 · Azure/azure-sdk-for-js

ramya-rao-a · 2019-08-03T03:58:29Z

This PR adds tracing to keyvault-keys library and is based on the work done by @sarangan12 in #4506

Each api in the convenience layer (index.ts) is updated to
- create a span based on the spanOptions in the request options passed by the user
- start & end the span before & after the call to the service
- set this new span as "parent" in the spanOptions in the request options so that
  - Any new span created in the pipeline (in the future) for this request gets the right parent
  - The right "parent" gets serialized in the header, so that any new span created on the service side gets the right parent.
The operation spec for each api is updated to serialize the contents of options.spanOptions in the header

sarangan12 · 2019-08-03T06:00:41Z

sdk/keyvault/keyvault-keys/src/core/keyVaultClient.ts

  path: "keys/{key-name}/create",
  urlParameters: [Parameters.vaultBaseUrl, Parameters.keyName0],
  queryParameters: [Parameters.apiVersion],
-  headerParameters: [Parameters.acceptLanguage],


This file needs few more additions in getDeletedKeysOperationSpec, importKeyOperationSpec, etc. Else, the span will not be serialized to the service.

Done with e2c529f

We need to speak with @daviwil and @schaabs to make sure that we can apply these changes to a future version of our core files, since they're auto-generated.

sarangan12 · 2019-08-03T06:01:38Z

I have tested with my sample with and without tracers and code looks fine. But some of the header parameters are missing and I added the comment on it. We must add them before release

sadasant

It looks good! Let's make sure we have a plan to have this updated in the auto-generated code. We also need this in secrets and in credentials, since they all share the same core.

sadasant · 2019-08-05T15:50:47Z

@ramya-rao-a & @sarangan12

Here's @schaabs 's reply to this PR:

[...] in general I would avoid manually editing the generated code as much as possible.

That being said I would imagine that inserting tracing around network calls would be ultimately inserted into autorest. So if the plan is to update autorest you would need to ensure autorest was updated in the same way.

I would suggest making the changes in autorest and regenerating though b/c that way you don't have to worry about differences ultimately.

I would talk to David, I believe he's done some work updating autorest to use core-http.

I'll come back with feedback from @daviwil (or he might too).

ramya-rao-a · 2019-08-05T16:51:29Z

@sadasant For Preview 3, we will definitely look into having this auto-generated.
For the Preview 2, the idea was to provide a proof of concept of how tracing would look like and what kind of plumbing would be needed. Therefore, we chose just keys and kept secrets, certificates untouched.

daviwil · 2019-08-05T17:12:18Z

@ramya-rao-a is correct, there isn't time to add this to the code generator for Preview 2 and all we really want is to validate the overall design anyway. I think hand-editing the code is fine in this case since we are in total control of when it gets regenerated in the future. We'll add some logic to autorest.typescript to generate this code in Preview 3, I don't think it will be that difficult to do so.

daviwil · 2019-08-05T17:20:47Z

sdk/keyvault/keyvault-keys/src/index.ts

+   */
+  private createSpan(methodName: string, requestOptions: RequestOptionsBase): Span {
+    const span = TracerProxy.getTracer().startSpan(methodName, requestOptions.spanOptions);
+    requestOptions.spanOptions = { ...requestOptions.spanOptions, parent: span };


It'd be nice if we could do this without directly manipulating the original requestOptions object, but it's fine for now.

daviwil · 2019-08-05T17:24:30Z

/azp run js - client - ci

azure-pipelines · 2019-08-05T17:24:36Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

daviwil · 2019-08-05T17:24:59Z

/azp run js - keyvault - ci

azure-pipelines · 2019-08-05T17:25:10Z

Azure Pipelines successfully started running 1 pipeline(s).

daviwil · 2019-08-05T17:25:29Z

/azp run js - core - ci

azure-pipelines · 2019-08-05T17:25:39Z

Azure Pipelines successfully started running 1 pipeline(s).

sophiajt · 2019-08-05T17:26:50Z

sdk/keyvault/keyvault-keys/src/index.ts

      delete unflattenedOptions.expires;
      delete unflattenedOptions.requestOptions;

+      const span = this.createSpan("createEcKeyMethod", unflattenedOptions);


This might be a bit of a nit, but why do these all end with "*Method"? It's a hunch, but I'd think we'd want the exact same name so that they can do a search for the symbol and find it.

Fixed with 64ea0ae

+1, method suffix is unnecessary, should match the symbol.

chradek · 2019-08-05T17:42:12Z

sdk/keyvault/keyvault-keys/src/index.ts

        unflattenedOptions
      );
+
+      span.end();


We also need to end the span if createKey throws an error, otherwise the span will never be closed.

Edit: This applies to all the places where we call start and stop around an awaited function.

Fixed with 53a6c66

ramya-rao-a · 2019-08-05T19:13:20Z

Serializing the entire spanOptions in the header is not desirable. We instead want to pass only the TraceContext in the header.

Therefore, updated this PR to revert changes related to headers in this PR with 139f4a3

We will look into creating a policy just for key vaults keys (since it already uses other custom policies) to do the needful in a separate PR

Add tracing to keyvault-keys library

6d74f87

ramya-rao-a requested review from daviwil, sadasant and sophiajt as code owners August 3, 2019 03:58

ramya-rao-a requested review from bterlson and sarangan12 August 3, 2019 03:58

createSpan method

99bff0e

sarangan12 reviewed Aug 3, 2019

View reviewed changes

ramya-rao-a mentioned this pull request Aug 4, 2019

SpanOptions in RequestOptionsBase to create span for tracing #4655

Merged

Update headers for all apis to allow serialization

e2c529f

ramya-rao-a mentioned this pull request Aug 4, 2019

Initial Prototype Code - Do NOT Merge #4506

Closed

ramya-rao-a added 2 commits August 4, 2019 22:46

Set the right parent when passing to service

3f319d9

Comment for createSpan

6e55042

sadasant reviewed Aug 5, 2019

View reviewed changes

daviwil approved these changes Aug 5, 2019

View reviewed changes

sophiajt reviewed Aug 5, 2019

View reviewed changes

chradek reviewed Aug 5, 2019

View reviewed changes

ramya-rao-a added 2 commits August 5, 2019 12:09

Revert changes related to headers

139f4a3

Merge remote-tracking branch 'origin/master' into keys-tracing

192858b

ramya-rao-a added 3 commits August 5, 2019 12:13

Add missing eol

ac84692

End span if request throws error

53a6c66

Remove method from span name

64ea0ae

sadasant approved these changes Aug 5, 2019

View reviewed changes

ramya-rao-a added 7 commits August 5, 2019 13:10

Add plugin type to tracer interface

e09c2c3

Update Tracer NoImpl

afb9cd5

Use readonly properties instead of getter

80e05cd

api update

b763e9b

Avoid updating spanoptions if noop tracer

0052bd1

Avoid updating spanoptions if no parent

dd75be3

Merge remote-tracking branch 'origin/master' into keys-tracing

1545c54

daviwil approved these changes Aug 5, 2019

View reviewed changes

ramya-rao-a merged commit c926735 into Azure:master Aug 5, 2019

ramya-rao-a deleted the keys-tracing branch August 5, 2019 20:28

Conversation

ramya-rao-a commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarangan12 commented Aug 3, 2019

Uh oh!

sadasant left a comment

Choose a reason for hiding this comment

Uh oh!

sadasant commented Aug 5, 2019

Uh oh!

ramya-rao-a commented Aug 5, 2019

Uh oh!

daviwil commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Aug 5, 2019

Uh oh!

azure-pipelines bot commented Aug 5, 2019

Uh oh!

daviwil commented Aug 5, 2019

Uh oh!

azure-pipelines bot commented Aug 5, 2019

Uh oh!

daviwil commented Aug 5, 2019

Uh oh!

azure-pipelines bot commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chradek Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ramya-rao-a commented Aug 3, 2019 •

edited

Loading

chradek Aug 5, 2019 •

edited

Loading