Add OTel semantic conventions for Outbound HTTP (span name and attributes) #63

pellared · 2021-02-26T12:34:44Z

Why

Attempt to implement @macrogreg idea described under #42

Extension points for trace/span conventions are needed so that we can support more than one model (e.g. OTel, DataDog, NewRelic, Splunk). We want to have it in a way that would maximize the possibility of having a common codebase. At the same time, we want to minimize potential conflicts if something "vendor" specific would need to be done on a fork.

What

Extension points for a semantic convention that is chosen at runtime via the OTEL_CONVENTION environmental variable. Currently defaults to Datadog.
Add OpenTelemetry Outbound HTTP semantic convention for span's operation name and attributes. Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md

I tried to make make the least changes in the existing code so that it is easy to cherry-pick this change by Datadog and other forks. All the new part is in dedicated classes to minimize potential conflicts.

TODO for this PR

Manual E2E testing
Double-checking with OTel specs and .NET SDK

Next PRs

Refactor test and use https://github.com/fluentassertions/fluentassertions
Address Add OTel semantic conventions for Outbound HTTP (span name and attributes) #63 (comment)
Add support for setting OTel status
Add support for adding OTel events (exceptions)
Try to address Add OTel semantic conventions for Outbound HTTP (span name and attributes) #63 (comment)

Questions

Do you want to use GitHub issues for making this ⬆️ stuff more transparent?
Any missing parts? E.g. do we have some docs where the environmental variables are described?
Do you want more descriptive comments in the code that describes the design decisions? Feel free to point me where such comments would be placed.

src/Datadog.Trace.ClrProfiler.Managed/Conventions/Convention.cs

src/Datadog.Trace.ClrProfiler.Managed/ScopeFactory.cs

src/Datadog.Trace.ClrProfiler.Managed/Conventions/OutboundHttpArgs.cs

pjanotti · 2021-03-01T03:42:02Z

@macrogreg @nrcventura @zacharycmontoya PTAL: it is in the initial stages and still small so easy to take a look and help guide @pellared here.

pellared · 2021-03-01T11:58:44Z

Refactored and improved a little. Description of the PR is updated.

pellared · 2021-03-01T12:02:12Z

src/Datadog.Trace/Tracer.cs

+            switch (Settings.Convention)
+            {
+                default:
+                    OutboundHttpConvention = new DatadogOutboundHttpConvention(this);


This bi-directional reference is ugly. I think it should be refactored.
The tracer is used this way:

string serviceName = _tracer.Settings.GetServiceName(_tracer, "http-client"); var scope = _tracer.StartActiveWithTags("http.request", tags: tags, serviceName: serviceName, spanId: args.SpanId);

However, it is still possible to test this code in ScopeFactoryTests.

hmmm now I see the reason of the .ctor with the tracer object.

It seems weird to me storing data from integrations (IOutboundHttpConvention
in this case) inside the tracer, this means that if we add new integrations that support conventions we need to modify the tracer as well?

Consider to expose only the ConventionType from the settings and each integration (that supports conventions) reads this value from the current Tracer.Instance and configure itself.

I prefer decouple the integrations from the tracer.

Relying on Tracer.Instace would make the code less maintainable/readable/testable.

I think that this proposal: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/63/files#r585368841 together with https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/63/files#r585374238 will address your main concerns.

The Tracer is the root object (aggregate) so it is normal that something "inside" would need to be changed when adding new stuff. Creating a facade would at least do it not "directly".

Maybe extracting some of the Tracer's functionalities to create spans would make it also a little cleaner, but TBH I am not sure if it is worth the effort. For sure I think it should not be done within this PR as it could cause too many merge conflicts with Datadog repo.

Good thanks for clarifying.

dszmigielski

LGTM

nrcventura

These changes look good to me. I only noticed a couple of minor spelling errors.

src/Datadog.Trace/Tagging/HttpTags.cs

pjanotti

I like the direction that it is going @pellared, one key question about how this grows to cover the other instrumentations.

src/Datadog.Trace/Configuration/TracerSettings.cs

src/Datadog.Trace/Configuration/ConfigurationKeys.cs

pjanotti · 2021-03-02T06:18:57Z

src/Datadog.Trace/Tracer.cs

@@ -253,6 +261,8 @@ public static Tracer Instance

        internal IDogStatsd Statsd { get; private set; }

+        internal IOutboundHttpConvention OutboundHttpConvention { get; }


I assume that in the end tracer has a single property for the semantic conventions instead of one for outbound HTTP a separate one for DB, etc. Right now it is only the first "target", correct?

In other words, what are your plans for it to grow?

Option 1. Have multiple fields
Option 2. Have a facade with multiple sematic conventions implementations

I would go with Option 1 and when we start having 3 interfaces then we should probably refactor it to Option 2.

I do not want to create a single gigantic interface. Even the OTEL specs have semantics conventions separated by protocol and I would prefer to design the code in a similar manner: https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/trace/semantic_conventions E.g. maybe we will combine HTTP Outbound and Inbound into a single interface.

test/Datadog.Trace.Tests/Tagging/TagsListTests.cs

src/Datadog.Trace/Conventions/IOutboundHttpConvention.cs

zacharycmontoya · 2021-03-02T15:47:35Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+
+        public OtelOutboundHttpConvention(Tracer tracer)
+        {
+            _tracer = tracer;


As you commented earlier, this reference is kind of ugly. The conventions are meant to differentiate spans right? Perhaps, we can have a Tracer create the scope first (not inside this class) and this class is only responsible for setting the tags or modifying the span, but that can all be done without the Tracer. That might require also identifying the operation name, but that could be a readonly property on this class

I did the way you suggest initially, but modifying the span's OperationName and setting the HttpTag's tag mapping after the object is created looked even worse to my eyes.

This is the problematic code:

tags = new HttpTags(tagKeys); // sets the tags mapping conventions string serviceName = _tracer.Settings.GetServiceName(_tracer, "http-client"); // I do not know what it is yet :) var scope = _tracer.StartActiveWithTags("http.request", tags: tags, serviceName: serviceName, spanId: args.SpanId); // this sets some stuff

I am not sure yet if we want to have a different ServiceName, OperationName for OTel and Datadog.

Maybe I will be able to refactor it if I simply know more about OTel and be more familiar with this repo 😉

Let me know if you think that this issue is a blocker.

No I don't think this is a blocker. Maybe later we can revisit this to make it cleaner, but anyways we will need to make a runtime decision based on how the Tracer is configured, and this accomplishes that 👍🏼

I'm missing something for sure, but, why not use Tracer.Instance instead? If the configuration is changed at runtime we cannot pick the new configuration. Also removes this .ctor.

It will be picked up. Here is why

HttpClientHandlerCommon invokes ScopeFactory.CreateOutboundHttpScope(Tracer.Instance, ...

ScopeFactory.CreateOutboundHttpScope invokes tracer.OutboundHttpConvention.CreateScope(args, out tags);

the OutboundHttpConvention usses Tracer passed via this

Not using Tracer.Instance makes it unit-testable, less tightly coupled and you see the dependency in the constructor. And it is easier to see the bi-directional dependency code smell.

Good thanks for clarifying.

src/Datadog.Trace/Tagging/HttpTags.cs

pellared · 2021-03-03T08:52:26Z

@zacharycmontoya I noticed that the ScopeFactory does not assign the http.request.headers.host tag. Is it OK for you? If you want it I can add it to this PR.

Reference:

public const string HttpRequestHeadersHost = "http.request.headers.host";

pellared · 2021-03-03T09:11:50Z

test/Datadog.Trace.ClrProfiler.Managed.Tests/OtelScopeFactoryTests.cs

+            public string HttpUrlTag;
+            public string HttpTargetTag;
+
+            public override string ToString()


I would prefer to use https://github.com/fluentassertions/fluentassertions

I can do it in the next PR

pellared · 2021-03-03T14:08:02Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+            string resourceUrl = UriHelpers.CleanUri(requestUri, removeScheme: true, tryRemoveIds: true);
+            scope.Span.ResourceName = $"{httpMethod} {resourceUrl}";
+
+            var integrationId = args.IntegrationInfo;
+            tags.InstrumentationName = IntegrationRegistry.GetName(integrationId);
+            tags.SetAnalyticsSampleRate(integrationId, _tracer.Settings, enabledWithGlobalSetting: false);


Double-check:
Do you want to keep these? They are not required by OTel but I do not think it hurts to have this stuff.

This is only used for Datadog so it should probably be removed

@pjanotti @nrcventura can you confirm, please?

Keep tag.InstrumentationName (it follow an OT convention)

My vote is to remove tags.SetAnalyticsSampleRate: it is a Datadog convention.

@pjanotti What about ResourceName? I think it is consumed only by SpanMessagePackFormatter and DatadogLogger. Currently, I remove it, but I can bring it back if needed.

pellared · 2021-03-03T14:09:53Z

src/Datadog.Trace/Conventions/DatadogOutboundHttpConvention.cs

+            scope.Span.Type = SpanTypes.Http;
+
+            tags.HttpMethod = httpMethod;
+            tags.HttpUrl = UriHelpers.CleanUri(requestUri, removeScheme: false, tryRemoveIds: false);


I removed the null-checks for requestUri and UpercaseInvariant for httpMethod.
Both looked unnecessary to me.
For application development, I tend to be a defensive programmer, but here (for auto instrumentation) I think that performance matters and we should not do any unnecessary stuff.

zacharycmontoya · 2021-03-03T21:04:14Z

Zipkin exporter misses the AdditionalTags

Yes, the ZipkinSerializer is failing. Currently we're not surfacing the TagsList.GetAdditionalTags except inside TagsList.WriteTags...Let me think on this one...

pellared · 2021-03-03T22:17:01Z

Zipkin exporter misses the AdditionalTags

Yes, the ZipkinSerializer is failing. Currently we're not surfacing the TagsList.GetAdditionalTags except inside TagsList.WriteTags...Let me think on this one...

@zacharycmontoya : AFAIK @RassK is going to take a look at it

pellared · 2021-03-03T22:18:36Z

@zacharycmontoya I noticed that the ScopeFactory does not assign the http.request.headers.host tag. Is it OK for you? If you want it I can add it to this PR.
Reference:
public const string HttpRequestHeadersHost = "http.request.headers.host";
Yeah that sounds good to me. I'm guessing we didn't have that because the project started with a OpenTracing conventions and that wasn't part of the spec

I will address it in the next PR, just to have this one focusing on how an OTel semantic convention can be applied to other places.

pellared · 2021-03-04T08:48:03Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+            otelTags.HttpScheme = uri.Scheme;
+            otelTags.HttpHost = uri.Authority;
+            otelTags.HttpTarget = uri.PathAndQuery + uri.Fragment;


TBH I am not sure if we need (and want) these.
Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md

Well these are not Required by the specs, maybe we can think later if we should have an optional flag setting to expose those keys, but by default only use the required one for performance reasons???

@tonyredondo I am OK with removing these and making an issue to add an optional flag setting to expose those additional attributes/tags.

@pjanotti @zacharycmontoya do you agree?

Sounds good to me

@pellared please, create an issue to add an option for the extra tags.

I have removed these attributes and created an issue #73

pellared · 2021-03-04T08:56:16Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+
+            var uri = args.RequestUri;
+            otelTags.HttpMethod = args.HttpMethod;
+            otelTags.HttpUrl = string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);


The OTel. .NET SDK uses OrginalString. However, this can cause leakage of auth data.
See: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L115
I feel like this is a little vuln in .NET SDK. Do you agree?

Agree we shouldn't use the OriginalString approach if this can contain auth data.

I asked a question on #otel-specification Slack channel: https://cloud-native.slack.com/archives/C01N7PP1THC/p1614854143044600

Good find @pellared definitely do not use OriginalString.

tonyredondo

Overall LGTM, I left some comments.

src/Datadog.Trace.ClrProfiler.Managed/ScopeFactory.cs

tonyredondo · 2021-03-04T09:18:30Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+
+        public OtelOutboundHttpConvention(Tracer tracer)
+        {
+            _tracer = tracer;


I'm missing something for sure, but, why not use Tracer.Instance instead? If the configuration is changed at runtime we cannot pick the new configuration. Also removes this .ctor.

tonyredondo · 2021-03-04T09:20:40Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+
+            var uri = args.RequestUri;
+            otelTags.HttpMethod = args.HttpMethod;
+            otelTags.HttpUrl = string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);


Agree we shouldn't use the OriginalString approach if this can contain auth data.

tonyredondo · 2021-03-04T09:27:59Z

src/Datadog.Trace/Tracer.cs

+            switch (Settings.Convention)
+            {
+                default:
+                    OutboundHttpConvention = new DatadogOutboundHttpConvention(this);


hmmm now I see the reason of the .ctor with the tracer object.

It seems weird to me storing data from integrations (IOutboundHttpConvention
in this case) inside the tracer, this means that if we add new integrations that support conventions we need to modify the tracer as well?

Consider to expose only the ConventionType from the settings and each integration (that supports conventions) reads this value from the current Tracer.Instance and configure itself.

I prefer decouple the integrations from the tracer.

tonyredondo · 2021-03-04T09:33:51Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+            otelTags.HttpScheme = uri.Scheme;
+            otelTags.HttpHost = uri.Authority;
+            otelTags.HttpTarget = uri.PathAndQuery + uri.Fragment;


Well these are not Required by the specs, maybe we can think later if we should have an optional flag setting to expose those keys, but by default only use the required one for performance reasons???

tonyredondo

LGTM

pjanotti

LGTM, small things for follow-ups.

src/Datadog.Trace/Conventions/OtelHttpTags.cs

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

pjanotti · 2021-03-04T17:03:51Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+
+            var uri = args.RequestUri;
+            otelTags.HttpMethod = args.HttpMethod;
+            otelTags.HttpUrl = string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);


Good find @pellared definitely do not use OriginalString.

pjanotti · 2021-03-04T17:06:32Z

src/Datadog.Trace/Conventions/OtelOutboundHttpConvention.cs

+            otelTags.HttpScheme = uri.Scheme;
+            otelTags.HttpHost = uri.Authority;
+            otelTags.HttpTarget = uri.PathAndQuery + uri.Fragment;


@pellared please, create an issue to add an option for the extra tags.

pellared

@pjanotti
I think this PR is ready for being merged.

pellared requested a review from a team as a code owner February 26, 2021 12:34

pellared marked this pull request as draft February 26, 2021 12:36

pellared changed the title ~~Add example extension point for span conventions~~ Example extension point for span conventions Feb 26, 2021

dszmigielski reviewed Feb 26, 2021

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed/Conventions/Convention.cs Outdated Show resolved Hide resolved

pjanotti reviewed Mar 1, 2021

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed/ScopeFactory.cs Outdated Show resolved Hide resolved

pjanotti reviewed Mar 1, 2021

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed/Conventions/OutboundHttpArgs.cs Outdated Show resolved Hide resolved

pellared requested a review from pjanotti March 1, 2021 10:28

pellared commented Mar 1, 2021

View reviewed changes

dszmigielski reviewed Mar 1, 2021

View reviewed changes

pellared changed the title ~~Example extension point for span conventions~~ Prepare extension point for span conventions Mar 1, 2021

pellared marked this pull request as ready for review March 1, 2021 13:41

pellared changed the title ~~Prepare extension point for span conventions~~ Prepare extension points for span conventions Mar 1, 2021

nrcventura reviewed Mar 1, 2021

View reviewed changes

src/Datadog.Trace/Tagging/HttpTags.cs Outdated Show resolved Hide resolved

src/Datadog.Trace/Tagging/HttpTags.cs Outdated Show resolved Hide resolved

pjanotti reviewed Mar 2, 2021

View reviewed changes

zacharycmontoya reviewed Mar 2, 2021

View reviewed changes

pellared requested review from pjanotti, nrcventura and zacharycmontoya March 2, 2021 19:23

zacharycmontoya reviewed Mar 2, 2021

View reviewed changes

src/Datadog.Trace/Tagging/HttpTags.cs Outdated Show resolved Hide resolved

pellared closed this Mar 3, 2021

pellared reopened this Mar 3, 2021

pellared commented Mar 3, 2021

View reviewed changes

pellared changed the title ~~Prepare extension points for span conventions~~ Add extension points for semantic conventions and implement OutboundHttp span sematics Mar 3, 2021

pellared changed the title ~~Add extension points for semantic conventions and implement OutboundHttp span sematics~~ Add extension points for semantic conventions and implement OutboundHttp span conventions Mar 3, 2021

pellared changed the title ~~Add extension points for semantic conventions and implement OutboundHttp span conventions~~ Add extension points for semantic conventions and implement it for OutboundHttp span name and attributes Mar 3, 2021

pellared changed the title ~~Add extension points for semantic conventions and implement it for OutboundHttp span name and attributes~~ Add OTel semantic conventions for Outbound HTTP (span name and attributes) Mar 3, 2021

pellared commented Mar 3, 2021

View reviewed changes

pellared added 3 commits March 4, 2021 07:23

Remove unused HttpTagsKeysMapping

fc9f3ba

Remove AnalyticsSampleRate and ResourceName

96b121d

Refine to use GetAdditionalTags

ecfb8aa

pellared requested a review from zacharycmontoya March 4, 2021 07:56

Improve string concatenation performance

4b96ffe

pellared commented Mar 4, 2021

View reviewed changes

Add test case with port

e06918d

pellared commented Mar 4, 2021

View reviewed changes

tonyredondo reviewed Mar 4, 2021

View reviewed changes

Make OutboundHttpArgs readonly

317936c

tonyredondo approved these changes Mar 4, 2021

View reviewed changes

pellared closed this Mar 4, 2021

pellared reopened this Mar 4, 2021

zacharycmontoya approved these changes Mar 4, 2021

View reviewed changes

pjanotti approved these changes Mar 4, 2021

View reviewed changes

pellared added 2 commits March 4, 2021 18:00

Remove optional OTel HTTP attributes

b333cad

Change tag key for HttpClientHandlerType for OTel

dba0901

pellared mentioned this pull request Mar 4, 2021

Add possibility to add optional OTel HTTP attributes #73

Closed

pellared commented Mar 4, 2021

View reviewed changes

nrcventura approved these changes Mar 4, 2021

View reviewed changes

Merge branch 'main' into otel-span-model

82dfb12

pjanotti merged commit 1861c27 into open-telemetry:main Mar 5, 2021

pellared mentioned this pull request Mar 8, 2021

Use FluentAssertions #76

Merged

pellared deleted the otel-span-model branch March 8, 2021 11:08

pellared mentioned this pull request Mar 17, 2021

Add support for 128bit traceid #72

Merged

zacharycmontoya added a commit to DataDog/dd-trace-dotnet that referenced this pull request Mar 24, 2021

Straight port of open-telemetry/opentelemetry-dotnet-instrumentation#63

411edb2

zacharycmontoya mentioned this pull request Mar 24, 2021

Add abstraction for Outbound HTTP span names and attributes DataDog/dd-trace-dotnet#1310

Closed

		@@ -253,6 +261,8 @@ public static Tracer Instance

		internal IDogStatsd Statsd { get; private set; }

		internal IOutboundHttpConvention OutboundHttpConvention { get; }

Add OTel semantic conventions for Outbound HTTP (span name and attributes) #63

Add OTel semantic conventions for Outbound HTTP (span name and attributes) #63

Conversation

pellared commented Feb 26, 2021 • edited Loading

Why

What

TODO for this PR

Next PRs

Questions

pjanotti commented Mar 1, 2021

pellared commented Mar 1, 2021

pellared Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dszmigielski left a comment

Choose a reason for hiding this comment

nrcventura left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharycmontoya Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

pellared Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharycmontoya commented Mar 3, 2021 • edited Loading

pellared commented Mar 3, 2021

pellared commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyredondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyredondo left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared left a comment

Choose a reason for hiding this comment

pellared commented Feb 26, 2021 •

edited

Loading

pellared Mar 1, 2021 •

edited

Loading

pellared Mar 4, 2021 •

edited

Loading

pellared Mar 2, 2021 •

edited

Loading

zacharycmontoya Mar 2, 2021 •

edited

Loading

pellared Mar 2, 2021 •

edited

Loading

pellared Mar 4, 2021 •

edited

Loading

zacharycmontoya commented Mar 3, 2021 •

edited

Loading