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.

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

zipkin: Handle older semconv #3794

Merged
merged 6 commits into from
Jan 9, 2024
Merged

Conversation

pellared
Copy link
Member

@pellared pellared commented Dec 15, 2023

Fixes #3793

Changes

Update OpenTelemetry to Zipkin Transformation to handle attributes from older semantic conventions in a backwards compatible way so that following attributes are (again) handled:

  • net.peer.name
  • net.host.name
  • net.sock.peer.addr & net.sock.peer.port
  • server.socket.domain
  • server.socket.address & server.socket.port

The backwards compatibility of a stable OpenTelemetry to Zipkin Transformation was broken by:

@tigrannajaryan
Copy link
Member

It is quite unfortunate that we referenced experimental semconv from a Stable doc. You are right that formal rules requires us to make changes to this Stable doc in a way that preserves backward compatibility.

At the same time it seems wrong to carry this duplicate information because we made a mistake in the past. I wonder if we have a way out here?

@pellared
Copy link
Member Author

pellared commented Dec 15, 2023

At the same time it seems wrong to carry this duplicate information because we made a mistake in the past. I wonder if we have a way out here?

I am not sure if it is really a "duplicate information". The intention is that Zipkin's remoteEndpoint is set if any of the listed attributes is present. The old versions are "ranked" below (have lower priority) than the attribute equivalents from newer semconv.

I think it may be even more important to handle the old semconv than peer.hostname and peer.address which is even "more legacy" (from OpenTracing). I guess the intention of the translation is to handle "as much as possible".

@tigrannajaryan
Copy link
Member

I am not sure if it is really a "duplicate information". The intention is that Zipkin's remoteEndpoint is set if any of the listed attributes is present. The old versions are "ranked" below (have lower priority) than the attribute equivalents from newer semconv.

Nevermind. I didn't realize it is a ranked choice of one, not all of those.

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

LGTM pending @chalin's input on links and how that interacts with the website.

@carlosalberto
Copy link
Contributor

Ping @chalin

@chalin
Copy link
Contributor

chalin commented Jan 9, 2024

Ping @chalin

I'm back! Looking at this now.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline for a new suggested pattern regex.

LGTM once the regex is updated per the comment.

.markdown_link_check_config.json Outdated Show resolved Hide resolved
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
@pellared
Copy link
Member Author

pellared commented Jan 9, 2024

@carlosalberto I think this PR is ready to be merged.

@carlosalberto
Copy link
Contributor

Hey @Oberon00

As @trask mentioned, the note above the table should cover the case (as it actually covers all the listed values, e.g. peer.service, server.address, etc):

If Zipkin SpanKind resolves to either SpanKind.CLIENT or SpanKind.PRODUCER
then the service SHOULD specify remote endpoint otherwise Zipkin won't treat the
Span as a dependency.

Please fill an issue as a followup in case you think we should add details about this. Hope that's fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zipkin exporter introduced backwards incompatible translation changes
10 participants