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

Bug: Both TelemetryChannels do not correctly handle AggregateException #1173

Closed
TimothyMothra opened this issue Jul 19, 2019 · 4 comments
Closed

Comments

@TimothyMothra
Copy link
Member

TimothyMothra commented Jul 19, 2019

If you are reporting bug/issue, please provide detailed Repro instructions.

Repro Steps

still investigating, don't have a repro yet

Breeze fixed their issues. Can test using this: https://badssl.com/

Actual Behavior

We aren't handling the AggregateException.
Instead, we log this message:

"Failed to send: One or more errors occurred.."

try
{
// send request
this.Send(telemetryItems, timeout).Wait();
}
catch (Exception e)
{
CoreEventSource.Log.FailedToSend(e.Message);
}

Send method returns a Task

private Task Send(IEnumerable<ITelemetry> telemetryItems, TimeSpan timeout)

Expected Behavior

We need to correctly handle the AggregateException.

Version Info

SDK Version : all
.NET Version : all
How Application was onboarded with SDK(VisualStudio/StatusMonitor/Azure Extension) :
OS : nuget
Hosting Info (IIS/Azure WebApps/ etc) : onprem

@TimothyMothra
Copy link
Member Author

TimothyMothra commented Jul 20, 2019

ServerTelemetryChannel has the same bug.

TranssmissionSender does not handle the AggregateException

try
{
TelemetryChannelEventSource.Log.TransmissionSendStarted(acceptedTransmission.Id);
responseContent = await acceptedTransmission.SendAsync().ConfigureAwait(false);
}
catch (Exception e)
{
exception = e;
}
finally

There is an if later that tries to handle HttpRequestException, but it's also not checking for AggregateException

if (responseContent == null && exception is HttpRequestException)

@TimothyMothra TimothyMothra changed the title Bug: InMemoryChannel does not correctly handle AggregateException Bug: Both TelemetryChannels do not correctly handle AggregateException Jul 20, 2019
@cijothomas
Copy link
Contributor

@MS-TimothyMothra Great catch! We really want to log most accurate error message if things are not working well, and fixing this further improves supportability.
Can you confirm that this issue is not affecting reliability (we are still doing retries as expected), but only our logging, and hence suportability is affected?

@pharring
Copy link
Member

@MS-TimothyMothra I don't think ServerTelemetryChannel has the bug because it does an await which unpacks the Exception.

@TimothyMothra
Copy link
Member Author

@cijothomas No behavior is affected, we're only logging incorrect or incomplete exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants