[BUG] ElasticsearchDiagnosticsSubscriber's Http listener is throwing excessive unnecessary exceptions.

[jrlost]

APM Agent version

The version of the Elastic.Apm nuget package used 1.26.0

Environment

Operating system and version: Windows & Linux

.NET Framework/Core name and version (e.g. .NET 4.6.2, NET Core 3.1.100) : Net Core 3.1, Net 6, Net 8

Application Target Framework(s) (e.g. net462, netcoreapp3.1): netcoreapp3.1, net6.0, net8.0

Describe the bug

A clear and concise description of what the bug is.

While reviewing traces, I noticed that there were a bunch of exceptions being thrown but not logged. Upon review, they all look like they are related to the ElasticsearchDiagnosticsSubscriber.

To Reproduce

Steps to reproduce the behavior:

1. Use elastic apm
2. Enable the ElasticsearchDiagnosticsSubscriber
3. Run a trace while using elastic search
4. Note the exceptions.

Expected behavior

No exceptions are thrown

Actual behavior

Lots of exceptions are thrown

Details:

Start here: https://github.com/elastic/apm-agent-dotnet/blob/main/src/instrumentations/Elastic.Apm.Elasticsearch/HttpConnectionDiagnosticsListener.cs#L48

https://github.com/elastic/apm-agent-dotnet/blob/main/src/Elastic.Apm/Api/Http.cs#L41

https://github.com/elastic/apm-agent-dotnet/blob/main/src/Elastic.Apm/Helpers/Sanitization.cs#L74

Possibly change

internal static Uri Sanitize(this Uri uri)
{
    if (string.IsNullOrEmpty(uri.UserInfo))
        return uri;

    var builder = new UriBuilder(uri)
    {
        UserName = Consts.Redacted,
        Password = Consts.Redacted
    };
    return builder.Uri;
}

to

internal static Uri Sanitize(this Uri uri)
{
    if (!uri.IsAbsoluteUri || string.IsNullOrEmpty(uri.UserInfo))
        return uri;

    var builder = new UriBuilder(uri)
    {
        UserName = Consts.Redacted,
        Password = Consts.Redacted
    };
    return builder.Uri;
}

or if it needs to function even more closely where the out of TrySanitizeUrl becomes null, could do

internal static Uri Sanitize(this Uri uri)
{
    if (!uri.IsAbsoluteUri)
        return null;

    if (string.IsNullOrEmpty(uri.UserInfo))
        return uri;

    var builder = new UriBuilder(uri)
    {
        UserName = Consts.Redacted,
        Password = Consts.Redacted
    };
    return builder.Uri;
}

internal static bool Sanitize(Uri uri, out string result)
{
    try
    {
        var sanitized = uri.Sanitize();

        if (sanitized == null)
        {
            result = null;
            return false;
        }

        result = sanitized.ToString();
        return sanitized != uri;
    }
    catch
    {
        result = null;
        return false;
    }
}

Regardless, using this try/catch for common flows feels dirty and is no doubt adding unnecessary contention.

On the flipside of this, I don't know if relative URI set in the HttpConnectionDiagnosticsListener in the ElasticsearchDiagnosticsSubscriber is even correct. I don't have enough understanding of the codebase to understand why it would be passing the PathAndQuery and not the full URI.

span.Context.Http = new Http
{
    Method = requestData.Method.GetStringValue(),
    Url = requestUri?.PathAndQuery
};

Anyways, I'd appreciate any help in getting this cleaned up. Thanks

[jrlost]

Curiously, this error was actually noted originally here: https://github.com/elastic/apm-agent-dotnet/issues/1110, so it's been out there for a while.