-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
[eventhubs] - Upgrade Event Hubs to core-tracing preview.14 #20240
Conversation
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
091b4ab
to
41702c0
Compare
API changes have been detected in API changes - parentSpan?: Span | SpanContext; |
026e10f
to
e12d078
Compare
API changes have been detected in API changes + readonly messageState: "x-opt-message-state"; |
API changes have been detected in |
API changes have been detected in API changes + export interface AdditionalPolicyConfig {
+ policy: PipelinePolicy;
+ position: "perCall" | "perRetry";
+ }
+ export declare function authorizeRequestOnClaimChallenge(onChallengeOptions: AuthorizeRequestOnChallengeOptions): Promise<boolean>;
+ additionalPolicies?: AdditionalPolicyConfig[];
- export declare type RawResponseCallback = (rawResponse: FullOperationResponse, flatResponse: unknown) => void;
+ export declare type RawResponseCallback = (rawResponse: FullOperationResponse, flatResponse: unknown, error?: unknown) => void; |
API changes have been detected in |
API changes have been detected in |
API changes have been detected in |
191e6e0
to
1ec1f3f
Compare
Check enforcer isn't being updated but here's the build: https://dev.azure.com/azure-sdk/public/_build/results?buildId=1353756&view=results And the live test run: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1353821&view=results |
/azp run js - event-hubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
* | ||
* @internal | ||
*/ | ||
export function convertTryAddOptionsForCompatibility(tryAddOptions: TryAddOptions): TryAddOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this does if I understand it correctly is that it supports a few obsolete span parenting options.
In general our policy has been that it's unnecessary to keep backwards compat for tracing options, which is what this function does, at least while tracing is in preview. parentSpan
hasn't been a thing in quite some time, neither has spanOptions, and we rely exclusively on tracingContext throughout our code. I think it's safe to file this away but if you strongly disagree let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes here! My only dream (if it's not too messy) is a way to trim the Spans off the public shape of batch so we don't need to enforce them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops I didn't see that _messageSpanContexts
got removed here: 6b7e478
So I'm good now!
d0b856b
to
ce48e0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. 🎉
Just asking a few questions/doubts, might be dumb, asking anyway. 😎
ce48e0d
to
fdb0838
Compare
Can we have a changelog entry please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for the simplified testing!
fdb0838
to
3fab7f0
Compare
Added - would love some feedback if there's changes I should make |
3fab7f0
to
24832f7
Compare
### Packages impacted by this PR @azure/data-tables ### Issues associated with this PR - #20213 - #5607 ### Describe the problem that is addressed by this PR Now that @azure/core-tracing@preview.14 is out, and hopefully the last version before GA, we need to upgrade a few packages in order to dogfood both the upgrade experience and the usage of these packages with the new instrumentation package. My goal was to upgrade one AMQP package and a few HTTP packages in addition to core-rest-pipeline to collect feedback. Upgrading Data Tables now allows us to start using the new APIs in a client package. ### Provide a list of related PRs _(if any)_ - #20240
Packages impacted by this PR
@azure/event-hubs
@azure/test-utils
Issues associated with this PR
#20213
Describe the problem that is addressed by this PR
Now that @azure/core-tracing@preview.14 is out, and hopefully the last version before GA, we need to upgrade a few packages in order to dogfood both the upgrade experience and the usage of these packages with the new instrumentation package.
My goal was to upgrade one AMQP package and a few HTTP packages in addition to core-rest-pipeline to collect feedback.
Upgrading EventHubs now allows us to start using the new APIs in a client package.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Haven't decided if this should be merged or if we should just publish to NPM under a custom tag in order to allow customers to dogfood. I know there's an internal IoT Hub engineer that is interested in helping out so we just need something for them to try.
Right now though I am leaning towards merging this in.
I also decided to dump all the
parentSpan
legacy stuff - there really isn't a need to maintain tracing backwards compat with an option that has not been used in over 6 months.Are there test cases added in this PR? (If not, why?)
Lots and lots
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists