Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add namespace to server and peer name across namespaces #1247

Merged
merged 3 commits into from
Oct 31, 2024
Merged

add namespace to server and peer name across namespaces #1247

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb154d2
add namespace to server and peer name across namespaces in name resolver
esara Oct 11, 2024
2e03230
only add namespace to client and server address for client and server…
esara Oct 30, 2024
8ee7346
Merge branch 'main' into k8snamespace
grcevski Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/export/otel/traces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,17 @@ func (tr *tracesOTELReceiver) acceptSpan(span *request.Span) bool {
func traceAttributes(span *request.Span, optionalAttrs map[attr.Name]struct{}) []attribute.KeyValue {
var attrs []attribute.KeyValue

// add namespace to server and peer name across namespaces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@esara esara Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if span.OtherNamespace != "" && span.OtherNamespace != span.ServiceID.Namespace &&
// in Kafka events we never get the client, so we hijack the OtherNamespace for the clientId
span.Type != request.EventTypeKafkaServer && span.Type != request.EventTypeKafkaClient {
if span.IsClientSpan() {
span.HostName = request.SpanHost(span) + "." + span.OtherNamespace
} else {
span.PeerName = request.SpanPeer(span) + "." + span.OtherNamespace
}
}

switch span.Type {
case request.EventTypeHTTP:
attrs = []attribute.KeyValue{
Expand Down
Loading