HttpClient: Invoke Enrich when SocketException = HostNotFound #3407

alanwest · 2022-06-24T19:27:40Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

codecov · 2022-06-24T19:35:54Z

Codecov Report

Merging #3407 (d989284) into main (b7a3a83) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3407      +/-   ##
==========================================
- Coverage   86.39%   86.21%   -0.19%     
==========================================
  Files         258      258              
  Lines        9259     9254       -5     
==========================================
- Hits         7999     7978      -21     
- Misses       1260     1276      +16

Impacted Files	Coverage Δ
...tp/Implementation/HttpHandlerDiagnosticListener.cs	`73.17% <100.00%> (+4.20%)`	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	`50.00% <0.00%> (-28.58%)`	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	`59.09% <0.00%> (-18.19%)`	⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs	`85.00% <0.00%> (-15.00%)`	⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs	`72.72% <0.00%> (-13.64%)`	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	`91.20% <0.00%> (-3.30%)`	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	`96.87% <0.00%> (-0.79%)`	⬇️

cijothomas

LGTM with a changelog entry as this is user facing change

reyang · 2022-06-24T20:09:09Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

                        }
                    }
-
-                    if (exc.InnerException != null)
+                    else if (exc.InnerException != null)


The flow seems a bit weird. Consider moving the exc.InnerException is SocketException exception check inside exc.InnerException != null?

yup.
Also there is no need of a Switch case... as there is a single check..

So I moved the check, because I agree the else if was wonky. Though, yes as the code currently stands, there's zero need to explicitly check for the SocketException. It seems there was once some thought that we'd set the status description differently based on the type of exception, but clearly that was not actually happening in practice. So I guess I don't know what the original intent was here 😆.

I'll see if the history can shed any light on this...

Ah ha ok, all of this dates back to the time that Span.Status was more than just OK, ERROR, or UNSET

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Dependencies/Implementation/HttpHandlerDiagnosticListener.cs

Lines 161 to 177 in 1ed4a28

if (exc is HttpRequestException)

{

if (exc.InnerException is SocketException exception)

{

switch (exception.SocketErrorCode)

{

case SocketError.HostNotFound:

activity.SetStatus(Status.InvalidArgument.WithDescription(exc.Message));

return;

}

}

if (exc.InnerException != null)

{

activity.SetStatus(Status.Unknown.WithDescription(exc.Message));

}

}

I simplified it here 0cb81ea.

But still raises some questions... should we not be setting the status to ERROR when the exception is not an HttpRequestException?

…t/opentelemetry-dotnet into alanwest/http-client-onexception

alanwest · 2022-06-24T20:41:43Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

-                    {
-                        activity.SetStatus(Status.Error.WithDescription(exc.Message));
-                    }
+                    activity.SetStatus(Status.Error.WithDescription(exc.Message));


I guess this kind of broadens the scope where we record an error status + description to also include HttpRequestException where InnerException is null... seems kinda odd we'd limit it in this way.

* Added Jaeger Propagator to Opentelemetry.Extensions.Propagators (#3309) * Remove unnecessary bullet in CHANGELOG.md (#3352) Co-authored-by: Cijo Thomas <[email protected]> * Fix OTLP test (#3357) * Show that test is not doing what you might think it does * More asserts the merrier * Show this little test that it has potential * improve test coverage: InMemoryExporter & IDeferredMeterProviderBuilder (#3345) * [SDK] Circular buffer tweaks + cpu pressure test (#3349) * CircularBuffer tweaks and cpu pressure test. * Switch to Volatile.Read. * Perf tweaks. * Remove race check in debug after doing more testing. Co-authored-by: Cijo Thomas <[email protected]> * Fix event name logic + support null categoryname. (#3359) * In-memory Exporter: Buffer log scopes (#3360) * Buffer log scopes when using in-memory exporter. * CHANGELOG update. * Code review. * Tests. * CHANGELOG tweak. * SDK: Forward SetParentProvider to children of CompositeProcessor (#3368) * Examples: Fix ParentProvider not being set on MyFilteringProcessor (#3370) * Fix ParentProvider not being set on MyFilteringProcessor example. * Added XML comments. * Tweak. * Typo. * Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider (#3364) * Add helper ctors & forceflush on OpenTelemetryLoggerProvider. * CHANGELOG update. * Unit tests. * Code review. * Code review. * Tweak. * SDK: Nullable annotations for base classes & batch + shims to enable compiler features (#3374) * Nullable annotations and shims for SDK base classes & batch. * Target updates. * Remove System.Collections.Immutable ref. * ApiCompat attribute exclusions. * ASPNETCore instrumentation to populate httpflavor tag (#3372) * improve test coverage: InMemoryExporter SnapshotMetric (#3344) * Fix AspNetCore metrics to use correct value for http.flavor (#3379) * Fix AspNetCore metrics to use correct value for http.flavor * word better * Logs: Add LogRecordData (#3378) * Add LogRecordData and hook up to LogRecord. * CHANGELOG update. * Code review. * Remove SetHttpFlavor from Http instrumentations (#3381) * Asp.Net Core trace instrumentation to populate http schema tag (#3392) * Try asp.net core tests with inproc server (#3394) * Dedupe IsPackable (#3398) * Remove AspNet and AspNet.TelemetryHttpModule instrumentation projects (#3397) * Handle possible exception when initializing the default service name (#3405) * HttpClient: Invoke Enrich when SocketException = HostNotFound (#3407) * Add & use ConfigureResource API. (#3307) Co-authored-by: tyler jago <[email protected]> Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Timothy Mothra <[email protected]> Co-authored-by: Mikel Blanchard <[email protected]> Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]> Co-authored-by: Christian Neumüller <[email protected]>

HttpClient: Invoke Enrich when SocketException = HostNotFound

d39da57

alanwest requested a review from a team June 24, 2022 19:27

alanwest commented Jun 24, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs Outdated Show resolved Hide resolved

cijothomas approved these changes Jun 24, 2022

View reviewed changes

reyang reviewed Jun 24, 2022

View reviewed changes

reyang approved these changes Jun 24, 2022

View reviewed changes

alanwest and others added 5 commits June 24, 2022 13:10

Update changelog

ec9c0c2

Make it less wonky... but it still seems wonky

345d656

Merge branch 'main' into alanwest/http-client-onexception

11af651

Simplifiy setting of span status and description

0cb81ea

Merge branch 'alanwest/http-client-onexception' of github.com:alanwes…

54e8473

…t/opentelemetry-dotnet into alanwest/http-client-onexception

alanwest commented Jun 24, 2022

View reviewed changes

Remove unused using

d989284

cijothomas approved these changes Jun 24, 2022

View reviewed changes

cijothomas merged commit 98b4fde into open-telemetry:main Jun 24, 2022

alanwest deleted the alanwest/http-client-onexception branch June 24, 2022 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpClient: Invoke Enrich when SocketException = HostNotFound #3407

HttpClient: Invoke Enrich when SocketException = HostNotFound #3407

alanwest commented Jun 24, 2022

codecov bot commented Jun 24, 2022 •

edited

Loading

cijothomas left a comment

reyang Jun 24, 2022

cijothomas Jun 24, 2022

alanwest Jun 24, 2022

alanwest Jun 24, 2022 •

edited

Loading

alanwest Jun 24, 2022 •

edited

Loading

	if (exc is HttpRequestException)
	{
	if (exc.InnerException is SocketException exception)
	{
	switch (exception.SocketErrorCode)
	{
	case SocketError.HostNotFound:
	activity.SetStatus(Status.InvalidArgument.WithDescription(exc.Message));
	return;
	}
	}

	if (exc.InnerException != null)
	{
	activity.SetStatus(Status.Unknown.WithDescription(exc.Message));
	}
	}

HttpClient: Invoke Enrich when SocketException = HostNotFound #3407

HttpClient: Invoke Enrich when SocketException = HostNotFound #3407

Conversation

alanwest commented Jun 24, 2022

codecov bot commented Jun 24, 2022 • edited Loading

Codecov Report

cijothomas left a comment

Choose a reason for hiding this comment

reyang Jun 24, 2022

Choose a reason for hiding this comment

cijothomas Jun 24, 2022

Choose a reason for hiding this comment

alanwest Jun 24, 2022

Choose a reason for hiding this comment

alanwest Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

alanwest Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2022 •

edited

Loading

alanwest Jun 24, 2022 •

edited

Loading

alanwest Jun 24, 2022 •

edited

Loading