open-telemetry / opentelemetry-php

The OpenTelemetry PHP Library
https://opentelemetry.io/docs/instrumentation/php/
Apache License 2.0
751 stars 186 forks source link

Update Zipkin Exporter to conform to latest spec changes #295

Closed jodeev closed 2 years ago

jodeev commented 3 years ago

Our current implementation of the Zipkin Exporter needs an update to conform to the latest spec. See Zipkin exporter spec.

Work items:

Grunet commented 3 years ago

In comparing the SpanConverter for Zipkin here to the one in OTEL-JS (and not the spec directly just yet), it seems like most things are accounted for in the PHP version.

The only things I noticed on a quick first comparison were:

I'll probably want to take another slower pass through, and then maybe do the same with another language (Java? Python?) before trying to tackle the spec head on (I gave it an initial read but it felt fairly abstract not being familiar with the broader context)

EDIT: Per this table I just found documenting compliance to the specification by language, Java and Python seem like the best candidates to compare Zipkin support with

Grunet commented 3 years ago

Here's what I noticed from a quick skim of Java's implementation

bobstrecansky commented 3 years ago

@Grunet -

bobstrecansky commented 3 years ago

I'm sure Java's implementation strategy in your second comment is sound. Do you think it'd be terribly difficult to follow their patterns in our repo?

Grunet commented 3 years ago
* We do have a `SpanKind` implementation here: https://github.com/open-telemetry/opentelemetry-php/blob/f4124e08595bd911d1324e0eaee0593d7c9dc2d0/src/API/Trace/SpanKind.php - Is that different than what you were looking for?

Ah perfect! That at first glance looks like what the other languages have

* We also do have tags in the `SpanConverter`: https://github.com/open-telemetry/opentelemetry-php/blob/f4124e08595bd911d1324e0eaee0593d7c9dc2d0/src/Contrib/Zipkin/SpanConverter.php - are the functions in that file different than you were expecting for the tag implementation?  Those haven't been touched in a while, so it may just be that they were updated in the spec and we didn't update accordingly.

I think the commonality between JS and Java's "tags" handling that PHP didn't have was the little bit of extra handling/logic around the status code and error. The other pieces around "tags" handling seemed to line up afaict

Grunet commented 3 years ago

I'm sure Java's implementation strategy in your second comment is sound. Do you think it'd be terribly difficult to follow their patterns in our repo?

I'm no Java expert (and no PHP expert...) but nothing really looked far out of the realm of being adaptable from a first glance.

After I re-read the spec (and hopefully better grok it this time....) I think I should be able to adapt from what JS/Java/Python have into PHP, hopefully.

(I was also thinking it could be useful to actually learn more about Zipkin, never having used it myself, but this also doesn't seem super worth it, at least up front, so I probably won't do that for now)

Grunet commented 3 years ago

Some notes on a quick compare to Python. The explicit handling around max tag value length was the biggest difference I registered from Java (not seeing a mention of something like that in the spec, so maybe it's a Python specific quirk?)

Grunet commented 3 years ago

So I re-read the spec and a few things cropped up other than the things already mentioned above

Here are my raw notes:

Service Name

Span Kind

InstrumentationLibrary

Remote endpoint

- (Where is Zipkin -> OTLP done? I'm not seeing that anywhere in otel-php)

Attribute

Status

Events

Unit of Time

Request Payload

Considerations for Legacy (v1) Format

- This seems to have to do with converting from a Zipkin v1 span back into OTLP for,at, but like I mentioned above I'm not seeing any existing mentions of that in the otel-php codebase - And maybe I'm just not codesearching for the right things, but I'm not finding any evidence of "Zipkin to Otlp" logic elsewhere in open-telemetry

Grunet commented 3 years ago

I think I'll go ahead and get started on the easier, better understand things that need changing for PHP, and in parallel try to sort out what to do about these new concerns (the need for an inverse converter being the main one I think)

Grunet commented 3 years ago

After taking another quick glance at the Go implementations for Event attributes and Remote Endpoints, here's what I've learned

bobstrecansky commented 3 years ago

json_encode should probably be adequate to try and imitate the json.Marshal in Go. Those do seem like two main concerns.