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

Spec Conformance Review: Exporters/Zipkin #1604

Closed
5 tasks done
Aneurysm9 opened this issue Feb 25, 2021 · 12 comments · Fixed by open-telemetry/opentelemetry-specification#1600
Closed
5 tasks done

Spec Conformance Review: Exporters/Zipkin #1604

Aneurysm9 opened this issue Feb 25, 2021 · 12 comments · Fixed by open-telemetry/opentelemetry-specification#1600
Assignees
Labels
pkg:exporter:zipkin Related to the Zipkin exporter package
Milestone

Comments

@Aneurysm9
Copy link
Member

Aneurysm9 commented Feb 25, 2021

  • Review the OpenTelemetry to Zipkin Transformation and identify all normative requirements it contains.
  • Validate our implementation of the specification to conform or not. If it conforms, document the conformity here. If it does not open an Issue or PR track the work.
  • Review the Exporters/Zipkin section of the Spec Compliance Matrix.
  • If there are any items listed in matrix that are not required by the specification, need clarification, or are in conflict with the specification open a PR in the specification to correct it.
  • Update the matrix with the current state of Go (existing entries for Go should be ignored). If Go does not provide support for an item listed be sure to link to the Issue/PR tracking the work to support it.
@MadVikingGod
Copy link
Contributor

Feature Optional Go
Zipkin
Zipkin V1 JSON X ?
Zipkin V1 Thrift X ?
Zipkin V2 JSON * ?
Zipkin V2 Protobuf * ?
Service name mapping ?
SpanKind mapping ?
InstrumentationLibrary mapping ?
Boolean attributes ?
Array attributes ?
Status mapping ?
Error Status mapping ?
Event attributes mapping to Annotations ?
Integer microseconds in timestamps ?

* For each type of exporter, OTLP, Zipkin, and Jaeger, implementing at least one of the supported formats is required. Implementing more than one formats is optional.

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Service name

Zipkin service name MUST be set to the value of the resource attribute: service.name.
If no service.name is contained in a Span's Resource, it MUST be populated from the default Resource.

func getServiceName(attrs []attribute.KeyValue) string {
for _, kv := range attrs {
if kv.Key == semconv.ServiceNameKey {
return kv.Value.AsString()
}
}
// Resource holds a default value so this might not be reach.
return ""
}

We set the service name from the resource, but we default to "" instead of from the default resource. While the resource passed to with the span data should include the default resource attributes we need to explicitly default to this value.

Tracking with #1777

In Zipkin it is important that the service name is consistent for all spans in a local root.
Otherwise service graph and aggregations would not work properly.
OpenTelemetry doesn't provide this consistency guarantee.
Exporter may chose to override the value for service name based on a local root span to improve Zipkin user experience.

We do not currently do this. It is not a requirement, so not going to add this as a task.

Note, the attribute service.namespace MUST NOT be used for the Zipkin service name and should be sent as a Zipkin tag.

This is not used.

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

SpanKind

The following table lists all the SpanKind mappings between OpenTelemetry and Zipkin.

OpenTelemetry Zipkin Note
SpanKind.CLIENT SpanKind.CLIENT
SpanKind.SERVER SpanKind.SERVER
SpanKind.CONSUMER SpanKind.CONSUMER
SpanKind.PRODUCER SpanKind.PRODUCER
SpanKind.INTERNAL null must be omitted (set to null)

The underlying type in the Zipkin span model for the span kind is string. This means the that we have interpreted null mapping for the SpanKind.INTERNAL as "" or Undetermined in the model. Otherwise, our mapping straightforwardly follows this:

func toZipkinKind(kind trace.SpanKind) zkmodel.Kind {
switch kind {
case trace.SpanKindUnspecified:
return zkmodel.Undetermined
case trace.SpanKindInternal:
// The spec says we should set the kind to nil, but
// the model does not allow that.
return zkmodel.Undetermined
case trace.SpanKindServer:
return zkmodel.Server
case trace.SpanKindClient:
return zkmodel.Client
case trace.SpanKindProducer:
return zkmodel.Producer
case trace.SpanKindConsumer:
return zkmodel.Consumer
}
return zkmodel.Undetermined
}

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

InstrumentationLibrary

OpenTelemetry Span's InstrumentationLibrary MUST be reported as tags to Zipkin using the following mapping.

This is how we report this information:

if il := data.InstrumentationLibrary; il.Name != "" {
m[keyInstrumentationLibraryName] = il.Name
if il.Version != "" {
m[keyInstrumentationLibraryVersion] = il.Version
}
}

OpenTelemetry Zipkin
InstrumentationLibrary.name otel.library.name
InstrumentationLibrary.version otel.library.version

Currently we do not use these

const (
keyInstrumentationLibraryName = "otel.instrumentation_library.name"
keyInstrumentationLibraryVersion = "otel.instrumentation_library.version"
)

But, they are being addressed in #1688

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

OTLP -> Zipkin

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.
peer.service is the preferred attribute but is not always available.
The following table lists the possible attributes for remoteEndpoint by preferred ranking:

Rank Attribute Name Reason
1 peer.service OpenTelemetry adopted attribute for remote service.
2 net.peer.name OpenTelemetry adopted attribute for remote hostname, or similar.
3 net.peer.ip & net.peer.port OpenTelemetry adopted attribute for remote address of the peer.
4 peer.hostname Remote hostname defined in OpenTracing specification.
5 peer.address Remote address defined in OpenTracing specification.
6 http.host Commonly used HTTP host header attribute for Http Spans.
7 db.name Commonly used database name attribute for DB Spans.
  • Ranking should control the selection order.
    For example, net.peer.name (Rank 2) should be selected before http.host (Rank 6).
  • net.peer.ip can be used by itself as remoteEndpoint but should be combined with net.peer.port if it is also present.

Being addressed in #1688

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Attribute

OpenTelemetry Span Attribute(s) MUST be reported as tags to Zipkin.

This is how we map them:

for _, kv := range data.Attributes {
m[(string)(kv.Key)] = kv.Value.Emit()
}

Some attributes defined in semantic convention document maps to the strongly-typed fields of Zipkin spans.

Primitive types MUST be converted to string using en-US culture settings.
Boolean values MUST use lower case strings "true" and "false".

The "attributes".Value.Emit method is used to conform with this:

// Emit returns a string representation of Value's data.
func (v Value) Emit() string {
switch v.Type() {
case ARRAY:
return fmt.Sprint(v.array)
case BOOL:
return strconv.FormatBool(v.AsBool())
case INT64:
return strconv.FormatInt(v.AsInt64(), 10)
case FLOAT64:
return fmt.Sprint(v.AsFloat64())
case STRING:
return v.stringly
default:
return "unknown"
}
}

Array values MUST be serialized to string like a JSON list as mentioned in semantic conventions.

This is not done, but is being addressed in #1688

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Status

Span Status MUST be reported as a key-value pair in tags to Zipkin, unless it is UNSET.
In the latter case it MUST NOT be reported.

We do not filter out the UNSET status:

m["otel.status_code"] = data.StatusCode.String()

Being addressed in #1688

The following table defines the OpenTelemetry Status to Zipkin tags mapping.

Status Tag Key Tag Value
Code otel.status_code Name of the code, either OK or ERROR. MUST NOT be set if the code is UNSET.
Description error Description of the Status. MUST be set if the code is ERROR, use an empty string if Description has no value. MUST NOT be set for OK and UNSET codes.

Note: The error tag should only be set if Status is Error.
If a boolean version ({"error":false} or {"error":"false"}) is present, it SHOULD be removed.
Zipkin will treat any span with error sent as failed.

Not implemented correctly, but being addressed in #1688

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Events

OpenTelemetry Event has an optional Attribute(s) which is not supported by Zipkin.
Events MUST be converted to the Annotations with the names which will include attribute values like this:

"my-event-name": { "key1" : "value1", "key2": "value2" }

This is how we map these values:

func toZipkinAnnotations(events []trace.Event) []zkmodel.Annotation {
if len(events) == 0 {
return nil
}
annotations := make([]zkmodel.Annotation, 0, len(events))
for _, event := range events {
value := event.Name
if len(event.Attributes) > 0 {
jsonString := attributesToJSONMapString(event.Attributes)
if jsonString != "" {
value = fmt.Sprintf("%s: %s", event.Name, jsonString)
}
}
annotations = append(annotations, zkmodel.Annotation{
Timestamp: event.Time,
Value: value,
})
}
return annotations
}
func attributesToJSONMapString(attributes []attribute.KeyValue) string {
m := make(map[string]interface{}, len(attributes))
for _, attribute := range attributes {
m[(string)(attribute.Key)] = attribute.Value.AsInterface()
}
// if an error happens, the result will be an empty string
jsonBytes, _ := json.Marshal(m)
return (string)(jsonBytes)
}

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Unit of Time

Zipkin times like timestamp, duration and annotation.timestamp MUST be reported in microseconds as whole numbers.
For example, duration of 1234 nanoseconds will be represented as 1 microsecond.

The Go Zipkin model uses time.Time and time.Duration to represent time. Meaning, our mapping (from the same type) is just to pass these types. The specification does not appear applicable here.

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Request Payload

For performance considerations, Zipkin fields that can be absent SHOULD be omitted from the payload when they are empty in the OpenTelemetry Span.

For example, an OpenTelemetry Span without any Event should not have an annotations field in the Zipkin payload.

We do this for events:

if len(events) == 0 {
return nil
}

#1688 will also do this for tags:

https://github.com/open-telemetry/opentelemetry-go/pull/1688/files#diff-655880b4017f8eb43d67420fad745dba9673714e963b4c51e40df3bde316647eR187

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Considerations for Legacy (v1) Format

Zipkin's v2 json format was defined in 2017, followed up by a protobuf format in 2018.

Frameworks made before then use a more complex v1 Thrift or json format that notably differs in so far as it uses terminology such as Binary Annotation, and repeats endpoint information on each attribute.

Consider using V1SpanConverter.java as a reference implementation for converting v1 model to OpenTelemetry.

The span timestamp and duration were late additions to the V1 format. As in the code link above, it is possible to heuristically derive them from annotations.

We do not convert into this format (nor convert from it).

@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Feature Optional Go
Zipkin V1 JSON X -
Zipkin V1 Thrift X -
Zipkin V2 JSON * +
Zipkin V2 Protobuf * -
Service name mapping -
SpanKind mapping +
InstrumentationLibrary mapping -
Boolean attributes +
Array attributes -
Status mapping -
Error Status mapping -
Event attributes mapping to Annotations +
Integer microseconds in timestamps N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:exporter:zipkin Related to the Zipkin exporter package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants