add namespace to server and peer name across namespaces

[esara]

This change will append the namespace to the server or peer address if the destination does not match the resource namespace, running in k8s

addressing #1246

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.60%. Comparing base (d3b2240) to head (8ee7346).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/export/otel/traces.go	66.66%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1247      +/-   ##
==========================================
+ Coverage   80.58%   80.60%   +0.01%     
==========================================
  Files         141      141              
  Lines       14217    14226       +9     
==========================================
+ Hits        11457    11467      +10     
+ Misses       2221     2218       -3     
- Partials      539      541       +2     

Flag	Coverage Δ
integration-test	58.73% <33.33%> (-0.04%)	⬇️
k8s-integration-test	58.36% <66.66%> (-0.18%)	⬇️
oats-test	35.01% <33.33%> (-0.01%)	⬇️
unittests	52.57% <33.33%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mariomac]

Thank you for your contribution @esara!

We need to discuss whether this addition could break service graph metrics, as I'm afraid that adding the namespace only to the name of the remote part, could lead to some metrics like:

service_graph_client{client="client",server="server.namespace"}
service_graph_server{client="client.namespace",server="server"}

I'm afraid that different naming conventions for client and server could lead to breaking service graphs.

[grcevski]

I was thinking more about this issue, would it be acceptable if we only added the .namespace in the traces output? I'm worried that if we add it everywhere it will break how we expect the metrics to behave, especially since the span metrics expect the service name to be separated from service namespace.

I'm thinking we should only change:

			request.ClientAddr(request.SpanPeer(span)),
			request.ServerAddr(request.SpanHost(span)),

in func traceAttributes(span *request.Span, optionalAttrs map[attr.Name]struct{}) []attribute.KeyValue { in traces.go.

Would this work for you @esara ?

I this change will be more correct based on the spec, but everything that's based on the service name will remain intact.

[grcevski]

We can also make it consistent with the HTTP/gRPC/DB metrics by changing span_getters.go and adding similar logic in here:

	case attr.ClientAddr:
		getter = func(s *Span) attribute.KeyValue { return ClientAddr(SpanPeer(s)) }
	case attr.ServerAddr:
		getter = func(s *Span) attribute.KeyValue { return ServerAddr(SpanHost(s)) }

If you'd like, I can make a PR on top of yours as previously. Name resolver collects the information and we can just use it in those places to extend what the server address is (or peer).

[esara]

I first tried to change traceAttributes.go - if this is not what you meant, I can also try with span_getters.go

[grcevski]

Hi @esara,

So I think we need to be more specific with how we setup the Peer and the Host, because the meaning of the server address changes depending if we are doing a client or server span.

Essentially the logic is like this:

	if span.IsClientSpan() {
		clientAddr = request.SpanPeer(span) + "." + span.ServiceID.Namespace
		serverAddr = request.SpanHost(span) + "." + span.OtherNamespace
	} else {
		clientAddr = request.SpanPeer(span) + "." + span.OtherNamespace
		serverAddr = request.SpanHost(span) + "." + span.ServiceID.Namespace
	}

I would make helpers functions for these somewhere in traces.go and change to use the values derived by those Server and Peer helpers everywhere where we do this now:

			request.ClientAddr(request.SpanPeer(span)),
			request.ServerAddr(request.SpanHost(span)),

There are few of those case statements below in the code. I know it's more complicated, but in Kafka events we never get the client (since it's forever lost on the queue) and we hijack the Othernamespace for the clientId. We probably should fix that :).

[esara]

that is a good point only to change the server and the client address respectively I added a second commit to do that - but I kept it in the method (except for kafka spans), while this can be factored out, that would actually be more code than this... also the current request.SpanPeer/SpanHost are shared with metrics, so we would need a local method which in turns calls them - maybe better organized, but a bit more complicated to read

functionally is this correct?

[grcevski]

Yes, this works now, thanks! Based on what I can tell, you prefer that we don't add the additional "." + namespace when the services are in the same namespace, correct? I don't have a strong opinion on that, perhaps adding the .namespace always might be more consistent, but it's more data.

[esara]

yes, I prefer it this way, because it is less intrusive to existing experience/service names - if microservices mostly communicate with each other within the namespace, it was already correct/no change - and the reference to services elsewhere would still work (metrics, logs, etc...)

this will only change the behavior, where the remote server (or client) is not obviously in the same space as the resource

[grcevski]

LGTM! I think this is correct now. We probably should follow up with fixing the kafka othernamespace issue.

[grcevski]

Yes, this works now, thanks! Based on what I can tell, you prefer that we don't add the additional "." + namespace when the services are in the same namespace, correct? I don't have a strong opinion on that, perhaps adding the .namespace always might be more consistent, but it's more data.

[grcevski]

Great, let's see what @mariomac thinks now. I think we can address the kafka issue in a follow-up PR.

[mariomac]

LGTM

[grcevski]

Thanks again @esara for your contribution!!!