-
Notifications
You must be signed in to change notification settings - Fork 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
fix(issue#25182): eventhub and svervicebus processor client start span independently #26180
fix(issue#25182): eventhub and svervicebus processor client start span independently #26180
Conversation
Sorry, the former PR I rebase wrong. Here is a new PR for fixing issue #25182 |
@lmolkova please take a look. I added the test and fix the same logic in service bus sdk. |
public void testProcessorWithTracingEnabledWithoutDiagnosticId() throws InterruptedException { | ||
final Tracer tracer = mock(Tracer.class); | ||
final List<Tracer> tracers = Collections.singletonList(tracer); | ||
final int numberOfTimes = 5; |
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.
Overall looks good, just curious, why we need to send 5 times message, rather than just 1 time?
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 referred to the previous tests. Most of them send 5 or 10 times for test. I did the same times for safety. Send 1 time works too.
1, null, false, Duration.ofSeconds(10), Duration.ofMinutes(1), LoadBalancingStrategy.BALANCED); | ||
|
||
eventProcessorClient.start(); | ||
TimeUnit.SECONDS.sleep(10); |
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.
Sleep in UT is probably not desirable. But given other tests already doing this, I guess you are good.
To other reviews:
Does this kind of tests really depends on a real clock, or it can run on virtual time?
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.
a better alternative would be to use something like CountDownLatch - signal in event processing callback and await for it to be done
2680596
to
486287a
Compare
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: |
486287a
to
f88481e
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.
- Can you please update the CHANGELOG for both Service Bus and Event Hubs? This helps us not lose track of any bugs fixed.
- LGTM! Thanks.
Co-authored-by: Weidong Xu <[email protected]>
fix casing for operation id (Azure#26180)
processor can start span if events or messages come from upstream with opentelemetry is not enable
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines