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.

Sign up for GitHub

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

Jump to bottom

update HttpSemanticConventions for Instrumentation.Http (part2) #4639

Merged
merged 21 commits into from
Jul 13, 2023
+688 −16
Merged

update HttpSemanticConventions for Instrumentation.Http (part2) #4639

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c0b2e57
update HttpSemanticConventions for Instrumentation.Http (net fx)
TimothyMothra Jul 6, 2023
3397105
Merge branch 'main' into 4484_http2
TimothyMothra Jul 6, 2023
0bcfdf3
fix
TimothyMothra Jul 6, 2023
0768089
remove url.scheme as this is for server, not client
TimothyMothra Jul 7, 2023
977dbb3
Merge branch 'main' into 4484_http2
TimothyMothra Jul 7, 2023
d66f7f9
Merge branch 'main' into 4484_http2
TimothyMothra Jul 7, 2023
4d0a5c7
Merge branch 'main' into 4484_http2
TimothyMothra Jul 10, 2023
14ecda8
Merge branch 'main' into 4484_http2
vishweshbankwar Jul 11, 2023
29b64ef
tests
TimothyMothra Jul 11, 2023
51999c4
Merge branch 'main' into 4484_http2
TimothyMothra Jul 11, 2023
f9e7b14
remove blink line
TimothyMothra Jul 11, 2023
88f817e
Merge branch '4484_http2' of https://github.com/TimothyMothra/opentel…
TimothyMothra Jul 11, 2023
c2ee1dd
update test comment
TimothyMothra Jul 12, 2023
b91509e
refactor to use booleans instead of HasFlag
TimothyMothra Jul 12, 2023
b326f3c
update changelog
TimothyMothra Jul 12, 2023
80adf0d
remove extra tests
TimothyMothra Jul 12, 2023
e5a54f1
Merge branch 'main' into 4484_http2
TimothyMothra Jul 12, 2023
edcc16d
correct link to spec in comments and changelog
TimothyMothra Jul 13, 2023
d0ff7e3
update more comments
TimothyMothra Jul 13, 2023
36475e2
fix build error
TimothyMothra Jul 13, 2023
743a311
Merge branch 'main' into 4484_http2
utpilla Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ public void OnStartActivity(Activity activity, object payload)
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md
if (this.emitNewAttributes)
{
activity.SetTag(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme);
activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, HttpTagHelper.GetNameForHttpMethod(request.Method));
activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);
if (!request.RequestUri.IsDefaultPort)
Expand Down
1 change: 0 additions & 1 deletion ...OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ public override void OnEventWritten(string name, object payload)
if (this.emitNewAttributes)
{
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeHttpRequestMethod, HttpTagHelper.GetNameForHttpMethod(request.Method)));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeUrlScheme, request.RequestUri.Scheme));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version)));
tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeServerAddress, request.RequestUri.Host));

Expand Down
53 changes: 44 additions & 9 deletions src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using System.Runtime.CompilerServices;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Trace;
using static OpenTelemetry.Internal.HttpSemanticConventionHelper;

namespace OpenTelemetry.Instrumentation.Http.Implementation
{
Expand Down Expand Up @@ -96,16 +97,34 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A

if (activity.IsAllDataRequested)
{
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host);
if (!request.RequestUri.IsDefaultPort)
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md
if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be extracted into boolean variables to avoid checking them repeatedly. This can be done in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern used in the other libraries won't work here because this is a static class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I'm not sure why this class uses a static instance of the options.

However, you should still be able to extract them into static bool variables in this case and use them instead of checking if the enum has a flag repeatedly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just a static instance of the options, the entire class is static.

I changed the Options field to a property so I can use the setter to evaluate the booleans in this commit: b91509e

{
activity.SetTag(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port);
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host);
if (!request.RequestUri.IsDefaultPort)
{
activity.SetTag(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port);
}

activity.SetTag(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme);
activity.SetTag(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion));
}

activity.SetTag(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme);
activity.SetTag(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion));
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md
if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New))
{
activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method);
activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);
if (!request.RequestUri.IsDefaultPort)
{
activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port);
}

activity.SetTag(SemanticConventions.AttributeUrlFull, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion));
}

try
{
Expand All @@ -123,7 +142,15 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity)
{
if (activity.IsAllDataRequested)
{
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode));
if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old))
{
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode));
}

if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New))
{
activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode));
}

activity.SetStatus(SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode));

Expand Down Expand Up @@ -153,7 +180,15 @@ private static void AddExceptionTags(Exception exception, Activity activity)
{
if (wexc.Response is HttpWebResponse response)
{
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode);
if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old))
{
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode);
}

if (Options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New))
{
activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)response.StatusCode);
}

status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)response.StatusCode);
}
Expand Down
Loading