[eventhubs] - Upgrade Event Hubs to core-tracing preview.14#20240
[eventhubs] - Upgrade Event Hubs to core-tracing preview.14#20240maorleger merged 2 commits intoAzure:mainfrom
Conversation
ghost
added
the
|
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: |
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
091b4ab to
41702c0
Compare
|
API changes have been detected in API changes - parentSpan?: Span | SpanContext; |
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
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 |
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
191e6e0 to
1ec1f3f
Compare
maorleger
requested review from
HarshaNalluru,
bterlson,
deyaaeldeen,
jeremymeng,
mikeharder,
praveenkuttappan,
ramya-rao-a,
sadasant,
timovv,
witemple-msft and
xirzec
as code owners
maorleger
changed the title
|
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). |
There was a problem hiding this comment.
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
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
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.
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
d0b856b to
ce48e0d
Compare
HarshaNalluru
left a comment
HarshaNalluru
left a comment
There was a problem hiding this comment.
Looks good overall. 🎉
Just asking a few questions/doubts, might be dumb, asking anyway. 😎
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
ce48e0d to
fdb0838
Compare
|
Can we have a changelog entry please? |
jeremymeng
left a comment
jeremymeng
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for the simplified testing!
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
fdb0838 to
3fab7f0
Compare
Added - would love some feedback if there's changes I should make |
maorleger
force-pushed
the
upgrade-eventhubs
branch
from
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
6 participants
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
parentSpanlegacy 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