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.

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

Only methods returning a Task<Operation<T>> are being instrumented automatically #29140

Closed
heaths opened this issue Jun 7, 2022 · 7 comments
Closed
Labels
Azure.Core.TestFramework Client This issue points to a problem in the data-plane of the library.

Comments

@heaths
Copy link
Member

heaths commented Jun 7, 2022

The InstrumentResultInterceptor is only automatically intercepting and proxying methods that return a Task<Operation<T>>. This was discovered when async tests were completing in tends of milliseconds, while sync tests were taking seconds. For the Conversations SDK, that triggered the 5s local playback error.

To work around this we can manually instrument, but it'd be nice if this was automatic so we didn't have to condition code to instrument.

@heaths heaths added Client This issue points to a problem in the data-plane of the library. Azure.Core.TestFramework labels Jun 7, 2022
heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Jun 7, 2022
@JoshLove-msft
Copy link
Member

This is specific to SyncOnly tests. Most tests are written as async and will have their operations instrumented here. The UseSyncMethodsInterceptor runs after this to handle running the sync variant of tests. The operation instrumentation doesn't work for SyncOnly tests because the operation method doesn't return a Task.

@heaths
Copy link
Member Author

heaths commented Jun 9, 2022

IIRC, you had a reason why this wouldn't work but I wanted to ask it again here for posterity, but would it not be as easy as switching the order of the interceptors for instrumenting async operations (to bifurcate sync and async) and the one to instrument operations based on method signature?

heaths added a commit that referenced this issue Jun 14, 2022
* Convert to DPG with HLC models

* Ignore long sync LRO test

Caused by #29140 (seemingly)

* Add swagger transforms

Works around #29141 and #29143

* Convert to DPG

Also fixes #26379

* Update public APIs and documentation

* Resolve PR feedback

* Resolve offline feedback

* Update generated code
Yao725 referenced this issue in shreyks/azure-sdk-for-net Jul 6, 2022
…29542)

* Prepare Conversations Language Understanding SDK 1.1.0-beta.1

* Convert to DPG with HLC models
* Ignore long sync LRO test

  Caused by Azure#29140 (seemingly)

* Add swagger transforms

  Works around Azure#29141 and Azure#29143

* Convert to DPG

  Also fixes Azure#26379

* Update public APIs and documentation
* Resolve PR feedback
* Resolve offline feedback
* Update generated code

* Stop always recording authoring tests

* Update samples

* Update CHANGELOG for release
zhihaoxue pushed a commit to zhihaoxue/azure-sdk-for-net that referenced this issue Jul 27, 2022
…29144)

* Convert to DPG with HLC models

* Ignore long sync LRO test

Caused by Azure#29140 (seemingly)

* Add swagger transforms

Works around Azure#29141 and Azure#29143

* Convert to DPG

Also fixes Azure#26379

* Update public APIs and documentation

* Resolve PR feedback

* Resolve offline feedback

* Update generated code
@heaths
Copy link
Member Author

heaths commented Oct 20, 2022

I tried it, and it's not quite so simple as reversing the order. Some changes to the flow and method name calculation in the UseSyncMethodIntercepter got me further, but it's clear this is going to require more thought and careful evaluation. I have a scenario where this would be handy, but will just work around this for now by using WaitUntil.Started in non-SNIPPET code and disable recordings while I wait - perhaps something akin to what @m-nash suggested in Azure/azure-sdk-tools#2703.

sofiar-msft pushed a commit to sofiar-msft/azure-sdk-for-net that referenced this issue Dec 7, 2022
…29144)

* Convert to DPG with HLC models

* Ignore long sync LRO test

Caused by Azure#29140 (seemingly)

* Add swagger transforms

Works around Azure#29141 and Azure#29143

* Convert to DPG

Also fixes Azure#26379

* Update public APIs and documentation

* Resolve PR feedback

* Resolve offline feedback

* Update generated code
sofiar-msft referenced this issue in sofiar-msft/azure-sdk-for-net Dec 7, 2022
…29542)

* Prepare Conversations Language Understanding SDK 1.1.0-beta.1

* Convert to DPG with HLC models
* Ignore long sync LRO test

  Caused by Azure#29140 (seemingly)

* Add swagger transforms

  Works around Azure#29141 and Azure#29143

* Convert to DPG

  Also fixes Azure#26379

* Update public APIs and documentation
* Resolve PR feedback
* Resolve offline feedback
* Update generated code

* Stop always recording authoring tests

* Update samples

* Update CHANGELOG for release
@pallavit
Copy link
Contributor

@heaths do you think there is value in exploring a solution here?

@heaths
Copy link
Member Author

heaths commented Jan 3, 2024

Yes. For sync-only tests that use LROs, we need a way to make sure they are runnable.

@heaths heaths closed this as completed Jan 3, 2024
@heaths heaths reopened this Jan 3, 2024
Copy link

github-actions bot commented Jun 7, 2024

Hi @heaths, we deeply appreciate your input into this project. Regrettably, this issue has remained unresolved for over 2 years and inactive for 30 days, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

1 similar comment
Copy link

github-actions bot commented Jul 8, 2024

Hi @heaths, we deeply appreciate your input into this project. Regrettably, this issue has remained unresolved for over 2 years and inactive for 30 days, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core.TestFramework Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

3 participants