Unnecessary semicolon. And looks like all the tracers copied the getTraceIdAsHex() which also has a unnecessary semicolon.

This setStreamInfo will only be called when we finalize downstream span (Entry Span for Skywalking). I am not sure the peer address of Entry Span is the upstream host address ?

The peer address of Entry Span should be the downstream address, right? And the peer address of Exit Span should be the upstream address.
cc @wu-sheng

The peer address of Entry Span should be the downstream address, right? And the peer address of Exit Span should be the upstream address.

The peer of exit span is the key. The entry span's is optional actually.

Then, the peer address of Exit Span should be the upstream address, is that right?

Yes, it is correct.

@wu-sheng Get it, thanks.

@Shikugawa based on the above discussions, may be we can complete the peer address injection of Exit Span directly in the #20367.

We can inject the downstream address of the Entry Span here if you think it's necessary. But it should be unnecessary, because Envoy will set peer address tag for the downstream span.

    const auto& remote_address = stream_info.downstreamAddressProvider().directRemoteAddress();

    if (remote_address->type() == Network::Address::Type::Ip) {
      const auto remote_ip = remote_address->ip();
      span.setTag(Tracing::Tags::get().PeerAddress, remote_ip->addressAsString());
    } else {
      span.setTag(Tracing::Tags::get().PeerAddress, remote_address->logicalName());
    }

I don't think this is good to set peer address in exit span in injectContext due to the incompatibility of the name of the function. I think that it will be better as follows:

void setStreamInfo(const StreamInfo::StreamInfo& stream) {
  if (span_entity_->spanType() == Entry) {
    span_entity_->setPeer(tream_info.downstreamAddressProvider().directRemoteAddress()->addressAsString());
  } else {
    span_entity_->setPeer(tream_info.upstreamInfo()->upstreamHost()->address()->asString());
  }
}

And calls setStreamInfo before finishSpan is called.

Thanks for your reply. My hope is that this can be done with minimal code modification, minimal performance overhead, and avoiding duplicate field settings.

If we had done this directly in #20367, the most critical exit span peer injection might have been done with just one extra line of code. And there is no need to repeatedly obtain the upstream host from stream info, and the performance will be slightly better.

The method name doesn't really matter. Because peer of exit span is a unique concept of skywalking, it is unknown to the caller of the virtual method. So we only need to do this work in any suitable method implementation.

However, this is only my personal opinion after all, it is only for reference. As long as the implementation is clean and correct, we can find another relevant maintainer to review the PR, and his review opinion shall prevail. 😸

Since these operations have nothing to do with Header Injection, they complicate the code on the SkyWalking side. In addition, the performance impact of such operations is negligible. So I think I should define setStreamInfo(). Do you have any quick ideas? cc. @mattklein123 @lizan

Since these operations have nothing to do with Header Injection

The entry span's operation does. The parent endpoint in the header is the entry span's operation name(path).