Remove the peer service resolver logic from zipkin exporter

[rajkumar-rangaraj]

Package: OpenTelemetry.Exporter.Zipkin
More Information: https://github.com/open-telemetry/opentelemetry-dotnet/pull/6015/files#r1878832826

[devc007]

Hey! @rajkumar-rangaraj @CodeBlanch
Can you guide me little bit, i was reading the reference you have provided, and after reading a lot still, I am finding my self not going any where.
Zipkin is a trace visualization tool and peer service resolver deals with the question that tells us "which service communicate with which other service"?
So why do we want to remove peer service resolver logic?

[Kielek]

@devc007, it is an exporter. The only goal of this module/library is to just take provided data and pack it in the given format. Any data-modification should be done on the instrumentation libraries side.

[devc007]

@Kielek Thank you so much for your reply! I was waiting for this. Right now, I am just trying to solve an issue in a different organization. I will look at this again later. Sorry, I forgot the whole context, so I need to read through this issue again.
Anyway, thank you so much for your response.

[CodingwithKetan]

Hey! @Kielek and @rajkumar-rangaraj , I’d like to take on this issue. Would it be okay if I work on it?

[Kielek]

@CodingwithKetan, it will be great, looking for PR :- )

[CodingwithKetan]

Hey! @Kielek and @rajkumar-rangaraj,
From the conversation, I understood that we need to completely remove any reference to the PeerServiceResolver class from the Zipkin exporter. In other words, we would remove the reference to Shared.PeerServiceResolver within Zipkin.

However, I have a different viewpoint that I’d like to discuss. The PeerServiceResolver is primarily responsible for parsing and extracting peer service data (e.g., IP address, hostname), similar to how we extract other data like StatusCode and status description in the ZipkinActivityConversionExtensions class.

In scenarios where the activity had kind as client or producer, we need to create an instance of RemoteEndpoint, and for that, we require information such as net.peer.name, net.peer.port, net.peer.ip, and peer.service. Additionally, managing priority for the peer name while creating the RemoteEndpoint is also crucial.

I understand that this can be achieved using the following code:

ZipkinEndpoint? remoteEndpoint = null;

if (activity.Kind == ActivityKind.Client || activity.Kind == ActivityKind.Producer)
{
    // Try to resolve the 'peer.service' tag first.
    string? peerServiceName = activity.GetTagValue(SemanticConventions.AttributePeerService) as string;

    if (peerServiceName == null)
    {
        // If 'peer.service' is not explicitly available, construct it from other tags.
        string? hostNameOrIpAddress = activity.GetTagValue(SemanticConventions.AttributeNetPeerName) as string
                                          ?? activity.GetTagValue(SemanticConventions.AttributeNetPeerIp) as string;

        if (hostNameOrIpAddress != null)
        {
            // If port is available, include it in the constructed peer service name.
            long port = activity.GetTagValue(SemanticConventions.AttributeNetPeerPort) as long? ?? default;
            peerServiceName = port == default
                ? hostNameOrIpAddress
                : $"{hostNameOrIpAddress}:{port}";
        }
    }

    if (peerServiceName != null)
    {
        // Create a remote endpoint using the resolved or constructed peer service name.
        remoteEndpoint = RemoteEndpointCache.GetOrAdd((peerServiceName, default), ZipkinEndpoint.Create);
    }
}

However, I feel this might not be the most optimal approach. For example, if in the future a new exporter (e.g., "xyz") is introduced, and we need similar (not exactly but we need remote ip and port ) logic for handling remote endpoint-related information, this would result in code duplication. I'd like to avoid repeating this logic across multiple places.

Please let me know if I’m missing something, or if there’s a better way to handle this while adhering to the goal of keeping the exporter focused on formatting and packing data rather than modifying it.

Looking forward to your insights!

[Kielek]

Calculating remoteEndpoint based on the tags is valid and supported scenario. What is more, it probably needs to be extended: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.40.0/specification/trace/sdk_exporters/zipkin.md#otlp---zipkin

Adding new tags during the exporting phase is not.

I do not think that code duplication should be considered here. Specification affects Zipkin exporter only. When the new exporter occurs (I doubt, as OTLP is recommended way) we can refactor the code.