HTTP adapters: Correct span names to match convention

[johnduhart]

(I'm using the PR template proposed in #629)

Fix Issue (None).

Changes

From the HTTP Semantic Conventions:

Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}". Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

This adjusts the adapters to match accordingly.

Checklist

• I ran Unit Tests locally.

The PR will automatically request reviews from code owners and trigger CI build and tests.

[cijothomas]

Can you move reused strings to private consts?

[johnduhart]

Where should I put that? Just create a const in each listener or should they be shared across both?

[cijothomas]

Shared would be great as they are both in same project OpenTelemetry.Adapter.Dependencies.

Maybe introduce a Contants.cs class just for this constants, and later we can move all shared constants there.

[CodeBlanch]

How do you guys feel about introducing a cache so we're not allocating strings for each Http span we process? Something like this...

• New file $\opentelemetry\src\OpenTelemetry.Api\Trace\SpanHttpHelper.cs

// <copyright file="SpanHttpHelper.cs" company="OpenTelemetry Authors">
// Copyright 2018, OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System;
using System.Collections.Concurrent;
using System.Net.Http;

namespace OpenTelemetry.Trace
{
    /// <summary>
    /// A collection of helper methods to be used when building Http spans.
    /// </summary>
    public static class SpanHttpHelper
    {
        private static readonly ConcurrentDictionary<string, string> MethodOperationNameCache = new ConcurrentDictionary<string, string>();
        private static readonly ConcurrentDictionary<HttpMethod, string> HttpMethodOperationNameCache = new ConcurrentDictionary<HttpMethod, string>();
        private static readonly ConcurrentDictionary<HttpMethod, string> HttpMethodNameCache = new ConcurrentDictionary<HttpMethod, string>();

        private static readonly Func<string, string> ConvertMethodToOperationNameRef = ConvertMethodToOperationName;
        private static readonly Func<HttpMethod, string> ConvertHttpMethodToOperationNameRef = ConvertHttpMethodToOperationName;
        private static readonly Func<HttpMethod, string> ConvertHttpMethodToNameRef = ConvertHttpMethodName;

        /// <summary>
        /// Gets the OpenTelemetry standard operation name for a span based on its Http method.
        /// </summary>
        /// <param name="method">Http method.</param>
        /// <returns>Span operation name.</returns>
        public static string GetOperationNameForHttpMethod(string method) => MethodOperationNameCache.GetOrAdd(method, ConvertMethodToOperationNameRef);

        /// <summary>
        /// Gets the OpenTelemetry standard operation name for a span based on its <see cref="HttpMethod"/>.
        /// </summary>
        /// <param name="method"><see cref="HttpMethod"/>.</param>
        /// <returns>Span operation name.</returns>
        public static string GetOperationNameForHttpMethod(HttpMethod method) => HttpMethodOperationNameCache.GetOrAdd(method, ConvertHttpMethodToOperationNameRef);

        /// <summary>
        /// Gets the OpenTelemetry standard method name for a span based on its <see cref="HttpMethod"/>.
        /// </summary>
        /// <param name="method"><see cref="HttpMethod"/>.</param>
        /// <returns>Span method name.</returns>
        public static string GetNameForHttpMethod(HttpMethod method) => HttpMethodNameCache.GetOrAdd(method, ConvertHttpMethodToNameRef);

        private static string ConvertMethodToOperationName(string method) => $"HTTP {method}";

        private static string ConvertHttpMethodToOperationName(HttpMethod method) => $"HTTP {method}";

        private static string ConvertHttpMethodName(HttpMethod method) => method.ToString();
    }
}

• Usage for HttpClient:

            this.Tracer.StartActiveSpanFromActivity(SpanHttpHelper.GetOperationNameForHttpMethod(request.Method), activity, SpanKind.Client, out var span);

            if (span.IsRecording)
            {
                span.PutComponentAttribute("http");
                span.PutHttpMethodAttribute(SpanHttpHelper.GetNameForHttpMethod(request.Method));

• Usage for HttpWebRequest:

            this.Tracer.StartActiveSpanFromActivity(SpanHttpHelper.GetOperationNameForHttpMethod(request.Method), activity, SpanKind.Client, out var span);

            if (span.IsRecording)
            {
                span.PutComponentAttribute("http");
                span.PutHttpMethodAttribute(request.Method);

Kind of overkill but I think it should be a goal of OT to keep allocations to a minimum.

[cijothomas]

@CodeBlanch This is great suggestion I think! Can you create a separate issue with this proposal. This PR can proceed without it.
Also consider https://github.com/dotnet/designs/pull/98/files as some of the upcoming changes to Span may eliminate the need of this.

[johnduhart]

@CodeBlanch That's really cool, thanks for writing that. The conventions state that instrumentat "...MAY provide hooks to allow custom logic to override the default span name.", so we should look to incorporate that with an change to the options to allow for overriding. I'm still getting accustomed to contributing to the project so maybe I can tackle that in a follow up PR.

[CodeBlanch]

@johnduhart Opened #639. Feel free to take that on! Adding something on options to allow users to mess with the spans that are created seems like a good idea to me. Might need to go through spec first though.