-
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 Event Hub live test breaks #33559
Conversation
/azp run java - eventhubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
API change check API changes are not detected in this pull request. |
/azp run java - eventhubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - eventhubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - eventhubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - eventhubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - eventhubs - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
return span; | ||
if (ERROR_STATUS_MESSAGE.equals(statusMessage)) { | ||
return span.setStatus(StatusCode.ERROR); | ||
} else if (SUCCESS_STATUS_MESSAGE.equals(statusMessage)) { |
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.
@lmolkova Can you double check this is what you intended? I noticed in EventHubsTracer that it was passing in "success" as the default value, which in the old code would be a StatusCode.Error. The javadocs for OpenTelemetryTracer.end didn't specify this case though, so I wasn't sure my fix is what you wanted.
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.
not really, I got rid of "success" altogether here #33600. along with other changes
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 think this should be covered in #33600 which should fix the tests and also add a few improvements, but I like the comments and clean-ups in this one.
UPDATE: I updated #33600 to incorporate changes and fixes from this PR and also returned back "success"
for the time being. I hope to get your eyes on that PR and merge it to fix tests.
/** | ||
* Unit tests for {@link OpenTelemetryUtils}. | ||
*/ | ||
public class OpenTelemetryUtilsTests { |
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.
this one is tested on OpenTelemetryTracerTests
… from Event Hubs.
0fa8700
to
230f181
Compare
Closed in favour of #33600 |
* Update tracing to new core changes * incorporating improvements from #33559
Description
Fixes integration test failures and adds some documentation to where constants are location.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines