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.

Sign up for GitHub

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

Jump to bottom

Use OperationTracingOptions from core-tracing #8389

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Use OperationTracingOptions from core-tracing #8389

merged 4 commits into from
Apr 16, 2020

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Apr 16, 2020 •
edited
Loading

The interface OperationTracingOptions is being defined in multiple places where as we should have a single source of truth.

  • core-http
  • identity
  • the 4 storage libraries
  • event hubs
  • service bus
  • core-auth

This PR makes it such that we define the interface in @azure/core-tracing and use it from there.

Note: This PR does not touch core-auth because it doesnt depend on @azure/core-tracing. Also because the problem with it is being tracked in #8301 where the solution is most probably going to be in-lining rather than re-using

@ramya-rao-a
Copy link
Contributor Author

ramya-rao-a commented Apr 16, 2020 •
edited
Loading

@bterlson, @xirzec, @jeremymeng, @daviwil Any of you recall why we chose to have tracing options re-defined in identity and the storage libraries?

Also, if we are ok with this approach, then this change should go in before #7998. If nothing, it will decrease the number of files being touched in #7998

@xirzec
Copy link
Member

xirzec commented Apr 16, 2020

@bterlson, @xirzec, @jeremymeng, @daviwil Any of you recall why we chose to have tracing options re-defined in identity and the storage libraries?

If I recall correctly, it's because we never exported TracingOptions directly. Later I figured out the TS magic to reference an interface that is exported indirectly:

type OperationTracingOptions = OperationOptions["tracingOptions"];

But I didn't go back and fix this everywhere.

Also, if we are ok with this approach, then this change should go in before #7998. If nothing, it will decrease the number of files being touched in #7998

I am good with this change, I think it makes sense to define the interface in core-tracing. I would also like to have us fix the goofiness of core-auth having its own OperationOptions soon. 😅

@xirzec xirzec added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Apr 16, 2020
@ramya-rao-a ramya-rao-a marked this pull request as ready for review April 16, 2020 18:16
xirzec
xirzec approved these changes Apr 16, 2020
View reviewed changes
sdk/storage/storage-file-datalake/review/storage-file-datalake.api.md Outdated
@@ -96,7 +96,7 @@ export interface AccountSASSignatureValues {
}

// @public
export class AnonymousCredential extends Credential {
export class AnonymousCredential extends Credential_2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit strange? Was this there before your change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a problem with the latest version of the api extractor. Once #8400 is merged, I'll update this PR and these changes should go away. I manually updated the .md files, looks like I missed datalake :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 045ba66

@ramya-rao-a
Copy link
Contributor Author

@richardpark-msft Can you review the Event Hubs and the Service Bus side of things here?

jeremymeng
jeremymeng reviewed Apr 16, 2020
View reviewed changes
sdk/storage/storage-blob/src/StorageClient.ts
@@ -19,13 +19,6 @@ export interface CommonOptions {
tracingOptions?: OperationTracingOptions;
}

export interface OperationTracingOptions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to re-export it here so it's not a breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not here, re-export from index

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never mind, I added the export earlier this month in index.ts so it hasn't been released yet.

jeremymeng
jeremymeng approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storage changes Looks good

richardpark-msft
richardpark-msft approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, nothing major. Looks okay from SB and EH side.

sdk/eventhub/event-hubs/src/partitionPump.ts
@@ -10,7 +10,7 @@ import { PartitionProcessor } from "./partitionProcessor";
import { EventHubConsumer } from "./receiver";
import { AbortController } from "@azure/abort-controller";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're not using abortSignal here. Can we narrow the type down to the field you want?

Pick<OperationOptions, 'tracingOptions'> // or something of that nature?

Copy link
Member

@richardpark-msft richardpark-msft Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I used the vscode extension and it put the comment at the top. I'm talking about the createProcessingSpan() signature change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean createProcessingSpan() doesnt need abort signal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged #8407 for a follow up that will cover the above

sdk/servicebus/service-bus/review/service-bus.api.md Show resolved Hide resolved
bterlson
bterlson approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@bterlson bterlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API changes look good, +1 for this change too, will be nice to have core-tracing as the source of truth here.

@ramya-rao-a ramya-rao-a merged commit d42bfdf into Azure:master Apr 16, 2020
@ramya-rao-a ramya-rao-a deleted the tracing-options branch April 16, 2020 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardpark-msft richardpark-msft richardpark-msft approved these changes

@jeremymeng jeremymeng jeremymeng approved these changes

@xirzec xirzec xirzec approved these changes

@bterlson bterlson bterlson approved these changes

@chradek chradek Awaiting requested review from chradek

@daviwil daviwil Awaiting requested review from daviwil

@HarshaNalluru HarshaNalluru Awaiting requested review from HarshaNalluru

@jiacfan jiacfan Awaiting requested review from jiacfan

@sophiajt sophiajt Awaiting requested review from sophiajt

@ljian3377 ljian3377 Awaiting requested review from ljian3377

@sadasant sadasant Awaiting requested review from sadasant

@schaabs schaabs Awaiting requested review from schaabs

@vinjiang vinjiang Awaiting requested review from vinjiang

@willmtemple willmtemple Awaiting requested review from willmtemple

@XiaoningLiu XiaoningLiu Awaiting requested review from XiaoningLiu

Assignees
No one assigned
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ramya-rao-a @xirzec @bterlson @jeremymeng @richardpark-msft