Initial Prototype Code - Do NOT Merge #4506
Conversation
There was a problem hiding this comment.
Few top level observations
- The d.ts files shouldn't be committed into the repo. Use the tsconfig.json file from either the folder
sdk/templateor thecore-authpackage. This will make the d.ts files to be generated undertypesfolder which is ignored by git in our repo. - Please use the rollup config files and package.json in the
sdk/templatefolder or thecore-authpackage as reference for the newcore-tracingpackage for consistency - A link in the PR description to the gist that Brian had initially proposed will help
| ? options.parent.getSpan() | ||
| : options.parent | ||
| : undefined | ||
| : undefined; |
There was a problem hiding this comment.
What do we have to gain by doing a deep check like this vs
const parent = options && options.parent
I would imagine that the very fact that this OpenCensusTracePlugin has been instantiated would mean that the parent we are getting would be the right one.
There was a problem hiding this comment.
This is because OpenCensus does not confirm to the OpenTelemetry Standards. So, you cannot pass options.parent (which is of type OpenCensusSpanPlugin which implements Span) It needs the specific span object created by OpenCensus Library
There was a problem hiding this comment.
If I understand this correctly, then you are saying that we need options.parent.getSpan() and not options.parent
The parent constant in the above code still points to only options.parent though...
| type: { | ||
| name: "Object" | ||
| } | ||
| } |
There was a problem hiding this comment.
When do we use the spanOptionsV2 instead of spanOptions?
There was a problem hiding this comment.
Let me give you an example in getKey method, when you are calling this.client.getKey, you are passing the options value as parameter. But in getDeletedKey method, when you are calling this.client.getDeletedKey, you are passing options.requestOptions. In order to handle this difference we have spanOptions & spanOptionsV2. Essentially they are the same. Only the path value where to find the spanOptions is different.
| @@ -276,12 +278,21 @@ export class KeysClient { | |||
| delete unflattenedOptions.expires; | |||
| delete unflattenedOptions.requestOptions; | |||
|
|
|||
| const spanOptions = this.getSpanOptions(unflattenedOptions); | |||
There was a problem hiding this comment.
This won't get the span options because requestOptions has been deleted from the unflattenedOptions in the line before this line
| return options; | ||
| } | ||
|
|
||
| return { ...options }; |
There was a problem hiding this comment.
... only does the needful at the top most level. This will not help us we will still be using the spanOptions object set by the user
I have updated #4654 such that we use the spread operator at the right level
No description provided.