codefromthecrypt commented 7 years ago

This is a proposal for a simpler json representation for tracers to use when reporting spans. This is a simplification of scope from what was formerly known as Zipkin v2 #939

Scope

The scope is only write, and only json. For example, this doesn't imply dropping support for using the same span ID across client and server RPC calls. It does imply converting this to the existing model at the point of collection.

Format

The following is the description of the simplified fields. Note that in the case of traceId it is fixed width vs variable. For 64-bit trace ids, simply pad left with 0s.

{
  "traceId": 16or32lowerHexCharacters,
  "parentId": 16lowerHexCharactersOrAbsentForRoot,
  "id": 16lowerHexCharacters,
  "kind": enum(CLIENT|SERVER|Absent),
  "name": stringOrAbsent,
  "timestamp": uint53EpochMicrosOrAbsentIfIncomplete,
  "duration": uint53MicrosOrAbsentIfIncomplete,
  "localEndpoint": existingEndpointTypeOrAbsent,
  "remoteEndpoint": existingEndpointTypeOrAbsent,
  "annotations": [
    {"timestamp": uint53EpochMicros, "value": string},
    ...
  ],
  "tags": {
    string: string,
    ...
  },
  "debug": trueOrAbsent,
  "shared": trueOrAbsent
}

Span name and timestamps are allowed to be missing to support late data, which is an edge case when spans are reported twice due to timeouts. Like previous versions, an empty span or service name defers the decision (usually to the other side of a shared span).

This format applied to a typical client span would look like this:

{
  "kind": "CLIENT",
  "traceId": "00000000000000005af7183fb1d4cf5f",
  "parentId": "6b221d5bc9e6496c",
  "id": "352bff9a74ca9ad2",
  "name": "query",
  "timestamp": 1461750040359130,
  "duration": 63874,
  "localEndpoint": {
    "serviceName": "zipkin-server",
    "ipv4": "172.19.0.3",
    "port": 9411
  },
  "remoteEndpoint": {
    "serviceName": "mysql",
    "ipv4": "172.19.0.2",
    "port": 3306
  },
  "tags": {
    "sql.query": "select distinct foo from bar"
  }
}

Impact on tracers

Tracers are the primary customers of this format, and in doing so they can have a closer alignment between field names and common operations. For example, the following is a "turing complete" span api which has almost direct mapping to this format.

start()
name(String name)
kind(Kind kind)
remoteEndpoint(Endpoint endpoint)
annotate(String value)
tag(String key, String value)
finish()
flush() <-- for reporting spans that didn't start and finish locally

It is important to remind that this format does not in any way drop support for the existing one. So old tracers can continue to operate.

Impact on servers

Collectors will decode and convert the new spans into the existing span model, backfilling "cs" and "cr" annotations and the "sa" binary annotation to make it appear as if it were sent in the old format. In order to do so, they need to know how to read the format. Choosing only json makes this simpler.

For http, we can add an endpoint or accept a media type header more specific than json. We could also choose a heuristic route. Kafka and similar systems which don't have headers would need a heuristic approach.

A heuristic approach would be to look for new fields. For example, startTimestamp or finishTimestamp will always be present in spans, so this can be used to decide which parsing rules to apply.

FAQ

Why just json?

This effort is geared at making tracer development simpler, and having the least scope possible to accomplish that. While thrift was supported in the old model, and has its merits, it is not a great format for people getting started, and it is very hard to debug. Those looking for reduced size of data can compress the spans before they are sent. Those who really like thrift can use the old model.

Why not special-case the tracer's endpoint?

The tracer's endpoint (localEndpoint) doesn't change from operation to operation, so you could reasonably ask why this isn't uploaded separate from the span data. The reason is that it lowers the scope of work. Many tracers report by adding spans to a queue flushed occasionally to a message producer. By keeping each span self-contained, this type of logic can be reused with no changes at all.

Why not change the query api, too?

This format is simpler because it removes the need to add the endpoint of the tracer to each annotation or tag. This simplification is possible in tracers as they have a one-one relationship with a traced service. At query time, a span can include both a client and a server, so it cannot be represented in this format.

Why not just remove support for the old model?

939 started as an effort to change the semantics of spans to single-host, not just for upload, but also processing, storage and query. After a year of discussion, it is clear this is not a practical or pragmatic first step. Reasons include the benefits of the current model and a general lack of interest. This is a tactical alternative that provides many of the wins without the work of a big bang refactor.

What about async spans?

We recently started supporting async/one-way spans via "cs" -> "sr" annotations. Notice the span begins and ends with the request side of the RPC (there's no response sent by the server or received by the client). This translates to the new model as the following: clientSideSpan.start().flush() serverSideSpan.start().flush().

codefromthecrypt commented 7 years ago

cc @openzipkin/instrumentation-owners @openzipkin/core

bvillanueva-mdsol commented 7 years ago

@adriancole what does Absent mean in "kind"? What kind of use case it stands for? Will it mean that we put 'Absent' as the value or put nothing at all?

bvillanueva-mdsol commented 7 years ago

@adriancole We were talking about async traces with @jcarres-mdsol . Is it safe to assume that if finishTimestamp is empty with kind as Client, an async process is being started with that span?

codefromthecrypt commented 7 years ago

@adriancole https://github.com/adriancole what does Absent mean in "kind"? What kind of use case it stands for? Will it mean that we put 'Absent' as the value or put nothing at all?

sorry I was trying to hint you can either leave out the field "key" or set it to null.

Kind is used to indicate the span is a client or server RPC span. This helps in translation to the old format, and means the user doesn't need do add annotations like "cs" and "cr". It also makes obvious what remoteAddress is (if client, the remoteAddress is server)

local spans (ex ones that never leave the host), would leave kind out.

codefromthecrypt commented 7 years ago

We were talking about async traces with @jcarres-mdsol . Is it safe to assume that if finishTimestamp is empty with kind as Client, an async process is being started with that span?

The semantics of one-way spans are currently the same as RPC, except there's no response. This was the least efforts means we could come up with. There's an example of this in a recent pull request #1497

To do this in using the new format in a backwards compatible way, the key is being able to flush a span (report it to zipkin without attempting to calculate duration locally).

ex. span.kind(client).start() --> translates into a "cs" annotation if you flush at this point -- propagated to the server then.. ex. span.kind(server).start() --> translates into an "sr" annotation if you flush at this point

Let me know if this doesn't answer your question

bvillanueva-mdsol commented 7 years ago

Thanks for the clarification @adriancole

eirslett commented 7 years ago

Note that 64 bit integers are tricky in JavaScript, which doesn't have them. Interestingly, it's possible to represent the number of milliseconds since 1970 in only ~52 bits, while JavaScript can represent whole numbers up to 53 bits. (Just a coincidence) So it will silently work until we eventually encounter a "year 2000 problem". It might be safer to store timestamps as strings (possibly compact/encoded), even if that could impact performance?

codefromthecrypt commented 7 years ago

Remember, 64 bit integers are tricky in JavaScript, which doesn't have them. Interestingly, it's possible to represent the number of milliseconds since 1970 in only ~52 bits, while JavaScript can represent whole numbers up to 53 bits. (Just a coincidence) So it will silently work until we eventually encounter a "year 2000 problem". It might be safer to store timestamps as strings (possibly compact/encoded), even if that could impact performance?

I hear you, and thanks for reminding about this. String dates are indeed idiomatic. Worth re-stating the rationale, since this issue is fairly special case. For example, it is making a shortcut form of the existing model (and would need to port without lossiness to the old one)

Here's some things to think about.

Everything currently uses the epoch micros, so it directly translates across. For example, when querying back (or looking at stored data) you would see the exact numeric form reported).

Date formats usually don't have microsecond precision, either.

Using a date format opens up to people in the N languages with uncoordinated tracers putting in an incorrect format (which we would immediately unwind it back into an epoch time). We'd get all the fun with people accidentally putting wrong timezone etc which I'm sure many of us are familiar with.

On 53bits. that's a known constraint wrt timestamps (our ids don't use numeric form for this reason). Ex our current json parser guards on 53bits which if I'm not mistaken puts the Y2K problem at the year 2255. While that's a punt for sure, since data is usually retained at a few days, sometime between now and 2255 I'm sure we'd have a new overall format.

Status quo does include the typical problem of people making ad-hoc or new tracers accidentally put epochMillis where they meant epochMicros. This is very easy to detect and fix. Heck we could probably even log it since when this mistake occurs the timestamps would look like they are from 1970. In other words, we can help make developing easier by making a development mode of zipkin which would crash POST on upload lint problems, such as 1970 dates.

Thoughts?

codefromthecrypt commented 7 years ago

ps will change proposal to 53bit as that's a very good point regardless!

codefromthecrypt commented 7 years ago

@eirslett and anyone else just circling back.. would any of the below help on the date thing?

It is certainly the case that "timestamp" implies seconds since epoch. Would replacing "timestamp" with "micros" or adding a suffix clear up enough ambiguity to be worth its characters? rename startTimestamp to startMicros or startTimestampMicros rename finishTimestamp to finishMicros or finishTimestampMicros

codefromthecrypt commented 7 years ago

ps I had an offline request somewhere about the size impact of this. Spans that include more than one annotation or tag will be smaller than before due to de-duplication of the local endpoint.

It won't be as small as using single-character field names, but it isn't optimizing for that anyway.

The compression of json vs the same data compressed in thrift is an exercise we could do, but I don't think size is the major feature here, rather simplicity.

shakuzen commented 7 years ago

Would replacing "timestamp" with "micros" or adding a suffix clear up enough ambiguity to be worth its characters?

I personally think unambiguous field names are best, so startTimestampMicros and finishTimestampMicros sound good to me. This way, the field names largely document themselves.

I don't see much value in optimizing for size here, especially considering compression. The marginal performance that I presume would be gained would not be worth the loss in clarity. And if such performance were a significant factor, JSON is already the wrong choice.

codefromthecrypt commented 7 years ago

thinking about picking this up

basvanbeek commented 7 years ago

One thing to think about... maybe do this format in proto3 as it supports a canonical encoding in JSON so we have a simple roll your own JSON model but at the same time can handle higher performance binary encoding/decoding.

codefromthecrypt commented 7 years ago

One thing to think about... maybe do this format in proto3 as it supports a canonical encoding in JSON so we have a simple roll your own JSON model but at the same time can handle higher performance binary encoding/decoding.

interesting idea. regardless we'd want a json schema, too (ex as we sortof do in the openapi spec)

codefromthecrypt commented 7 years ago

So first wave will be implementing the collection side, and a storage impl for elasticsearch. This can flex as we review the implementation.

Note: We aim to reuse hex encoding of IDs as they exist today (in lower-hex). This makes sure existing log integration patterns don't break (ex logging usually adds trace ID to context and that is lower-hex which is same as json and headers). This is particularly useful in storage backends that contain both logging and tracing data (ex Elasticsearch).

Note part deux: usually, collectors receive single-host spans, but we'll need to address shared spans sooner or later. Kafka pipelines exist (theoretically) that send merged traces directly to storage (ex via zipkin-sparkstreaming). Also, people sometimes save off json (which has merged spans) and POST it later. While not a data format issue, it is an integration issue that will bite us similarly to how it bit stackdriver when a storage layer like ES stores single-host spans. https://github.com/GoogleCloudPlatform/stackdriver-zipkin/blob/master/translation/src/main/java/com/google/cloud/trace/zipkin/translation/SpanTranslator.java

I'll do my best to keep all operational notes like above close to the code, so that nothing is surprising.

SergeyKanzhelev commented 7 years ago

What do you think of making this JSON two-layered - envelope that has a basic correlation fields and versioned&typed payload. Something along the lines:

{
  "traceId": 32lowerHexCharacters,
  "parentId": 16lowerHexCharactersOrAbsentForRoot,
  "kind": enum(SPAN_CLIENT|SPAN_SERVER|SPAN_Local|ANNOTATION),
  "debug": trueOrAbsent,
  "data": 
    {
      "id": 16lowerHexCharacters,
      "name": string,
      "finishTimestamp": uint53EpochMicros,
      "localEndpoint": existingEndpointType,
      "remoteEndpoint": existingEndpointTypeOrAbsent,
      "tags": {
        string: string,
        ...
      },
      "annotations": [
        {"timestamp": uint53EpochMicros, "value": string},
        ...
      ],
    }
}

This way you can solve some scenarios like:

You can send annotation separately from span without the need to specify the end time:

{
  "traceId": 32lowerHexCharacters,
  "parentId": 16lowerHexCharactersOrAbsentForRoot,
  "kind": ANNOTATION,
  "debug": trueOrAbsent,
  "data": 
  {
      "name": string,
      "localEndpoint": existingEndpointType,
      "tags": {
        string: string,
        ...
      },

Some annotations can be forced out of span in future so they could be indexed and queried easier
Versioning of span types may be easier in future. Just encode version in kind field
Other types like traces/exceptions/etc. may be supported as a payload in future. Those types will be correlated with traces without the need to change UI and tools. If type is unknown - tool can simply show JSON

codefromthecrypt commented 7 years ago

What do you think of making this JSON two-layered - envelope that has a basic correlation fields and versioned&typed payload. Something along the lines:

First thoughts were that this design was to reduce nesting, not add to it :) That said, at least this nesting will not introduce a collection to search through. Will comment more below, and regardless appreciate your interest.

This way you can solve some scenarios like:

You can send annotation separately from span without the need to specify the end time:

in-flight work has both timestamps optional, as it was an oversight to make them mandatory (for tags, not annotations, but anyway yeah). Reason is that there are use cases where data happens after the fact (even if it is exceptional and usually related to timeouts). Adding another layer of nesting doesn't make this easier or harder, imho.

Some annotations can be forced out of span in future so they could be indexed and queried easier

yeah that was an oversight in the sketch, not the impl. It doesn't require a data envelope to handle.

Versioning of span types may be easier in future. Just encode version in kind field

The kind field is a shortcut currently used to eliminate bookend annotations of "cs" "sr" etc. That's why it is an enum, not a open type. Encoding other data into that field would not be supported. For versioning, I think it is useful to define a schema and possibly a proto3 mapping.

Other types like traces/exceptions/etc. may be supported as a payload in future.

We wouldn't remove fields, though possibly add ones. I would be unsurprised if we had an exception type at some point (in the span). There's no likelihood of defining a separate "trace" type in zipkin (as opposed to a list of spans). Regardless, there's no issue tracking future changes in json schema and/or proto3.

Those types will be correlated with traces without the need to change UI and tools. If type is unknown - tool can simply show JSON

There are clear benefits for having this a straight-forward data type, including natural objects when code is generated from it. Unknown fields within this type can be skipped, as they are now, but changing this from a data type to be a generic carrier is beyond the intent.

Data type currently fits the model of existing tracers who post some or all of a span as a unit. There's no expectation that they will start sending arbitrary data, and it is a bit early to guess how they would. Meanwhile, escalating this change from a data type to a trace-stained-carrier will put it at risk of not completing (again).

TL;DR; let's keep this as a data type and fork any discussion off about a coordinated trace-identifier scoped carrier as a separate issue or thread. Let's make sure there any evaluation of such is diversely bought-into as we don't have enough hands to maintain things that aren't.

SergeyKanzhelev commented 7 years ago

@adriancole thanks for clarification. My understanding of schema was wrong I thought only these fields marked "Absent" can be omitted:

"kind": enum(CLIENT|SERVER|Absent),
"remoteEndpoint": existingEndpointTypeOrAbsent,
"debug": trueOrAbsent

So I wasn't clear on how you can report a span name for the runaway annotation. The intend with proposed envelope was to extract fields strictly required for correlation one level up and put all situation-specific fields inside the data object. Using kind you can strongly type those data objects further.

Also that proposal didn't change any data formats (except extended up kind to be a string) so I thought it may be incorporated now and make versioning easier in future.

Anyway, I see your point that this proposal doesn't solve any major problems you have immediate plans for and creates unnecessary complexity and overhead.

I really like how this schema simplify semantic of span!

codefromthecrypt commented 7 years ago

@adriancole https://github.com/adriancole thanks for clarification. My understanding of schema was wrong I thought only these fields marked "Absent" can be omitted:

heh you are right about the absent part, just that I didn't update the issue after working on it yesterday and noticing the missing absent on the timestamps. Will do now!

codefromthecrypt commented 7 years ago

updated about absent

SergeyKanzhelev commented 7 years ago

What about the name? Will you be able to report it when you have remote annotation only? Or it will be just empty?

codefromthecrypt commented 7 years ago

good point. we can accept absent span name and use the same semantics as what we currently do for empty (which is defer the naming choice). updated

codefromthecrypt commented 7 years ago

ok made a tracking issue for this https://github.com/openzipkin/zipkin/issues/1644

codefromthecrypt commented 7 years ago

ran into a couple things in java binding.

64-bit trace IDs are in heavy use, so assuming only 128-bit encoding might be a mistake
- For example, many using B3 still use 64-bit (as the spec defines two encodings). Assuming 128-bit padding knowing this can interfere with tokenization defined there.
- Once other specs like Trace-Context exist, we can reconsider this
In java, code in most places use uint64s (longs) for convenience, we should keep this convention.
- if we switched to string for IDs in the java/structs we need to explicitly check length/format of the string.
- Also we'd have to modify code that currently exists for random generation, span processing, or trace contexts.

So, while the representation of IDs will be hex like before, we shouldn't arbitrarily pad left, at least not now. We should also keep the ID layout in java/structs as uint64s as this facilitates interop with existing code.

codefromthecrypt commented 7 years ago

I found another glitch. We currently still support sharing spans between the client and the server. Unless we store the fact that the span was shared, we break duration logic (because client duration is authoritative zipkin.io/pages/instrumenting.html). I'm tentatively including the shared status so that we don't lose this signal.

codefromthecrypt commented 7 years ago

So, coming up for air on #1651

SimpleSpan to existing Span is relatively straightforward (due to prior work). Splitting existing Span into SimpleSpan is very complex due to many things including:

looseness of Endpoint type (which permits null eventhough it isn't really permitted)
Endpoint instances sometimes mismatch in the same process (one has port one doesn't)
Sometimes by mistake people end up with more than 2 hosts in one span.

"simplespan native" storage (ex elasticsearch w/o nested queries) depends on us being able to split old spans into simplified ones, so the impact of this complexity is delay on this milestone. We can't reasonably expect to have sites with only "simplespan" reporter/collectors, so this is a reality vs a nice to have.

TL;DR; more effort to get through this, hopefully another day or two.

codefromthecrypt commented 7 years ago

another glitch :) so existing span store tests allow spans to be written with no annotation or endpoint (usually the result of a late flush or a bug, but still possible). Moreover, the thrift defined endpoint fields optional (eventhough it botches search). Long story short, we need to make "localEndpoint" optional, if nothing else but to help track down instrumentation bugs.

codefromthecrypt commented 7 years ago

@openzipkin/core calling attention to something as it is important in solidifying the new simpler model. I'd like to consider reverting to span.timestamp/duration just like the existing one does.

As far as I can research, the idea of changing span.timestamp/duration as we have now to span.startTimestamp/finishTimestamp came from me. When working on brave v4 one of things I ran into was that when recording, it is sometimes better to have two timestamps instead of one and a duration.

from an api pov, all numeric values are timestamps (so accidentally putting duration where timestamp should go is impossible)
from a data pov, you could receive partial information, like start/end in different patches (as done in async span)

What I didn't think about was that we have a duration query. If we choose to store two timestamps instead of a single timestamp and a duration (as we do now), then the duration query is more effortful. For example, you need to sneak a field or make a custom index from date math.

So, in hindsight, even if folks support a two timestamp (start/finish) approach for the api, it might be better to report a duration field like we do now. This makes duration query dead simple in document stores like Elasticsearch, and from a code POV subtracting timestamps before writing json is easy. Moreover, the two-host async span sharing a single span ID thing will eventually go away anyway.

anuraaga commented 7 years ago

Agree that duration queries across documents without a preprocessing step would be very difficult. Though now I need to catch up on what an async span is to understand the implication of going back to start/duration ;)

codefromthecrypt commented 7 years ago

@anuraaga status quo async span is where "cs" and "sr" are annotations in different docs with the same span ID (one in the sender and the other in the receiver).

Modeling that to "simplespan with duration" it would be one doc with a started, but unfinished kind=client span, and another doc with a started, but unfinished kind=server span both sharing the same span ID. unfinished means it has no duration set.

codefromthecrypt commented 7 years ago

ps on one-way I don't see any (more) impact than we have today. Right now, you can't tell the difference between something that is one-way vs an RPC that hasn't yet finished (except that good clients shouldn't return things unfinished!). Essentially, it is a nuance we could clarify later if needed, as extra data, and separate from this change.

codefromthecrypt commented 7 years ago

next one is for @openzipkin/elasticsearch

Tag keys usually have dots in them. In the current model it might look like this.

 "tags" {
    "http.method": "GET"
  }

We claim to support Elasticsearch 2.x and 5.x. Elasticsearch 2.0-2.3 do not support dots in a name, though 2.4, 5.x do.

2.4 was released Aug 2016 and works if run with ES_JAVA_OPTS=-Dmapper.allow_dots_in_name=true

This simple json format looks otherwise ideal for elasticsearch. For example, there are no nested queries needed anymore and possibly not even a special type for servicespan. Placing 2.0-2.3 out of scope for use of simplespan-native storage seems more reasonable to me than rewriting the json structure to work with that version range. I think we'll need to read the old format for a while anyway.

What do you think? Should we keep the above format knowing it doesn't work with ES 2.0-2.3? When thinking of this, note that Amazon's Elasticsearch 2.x is not at 2.4, yet (though it has a couple 5.x versions)

codefromthecrypt commented 7 years ago

sent a note to users, too as many won't otherwise see this note https://twitter.com/zipkinproject/status/888696518735052801

codefromthecrypt commented 7 years ago

had a quick chat esp w/ @basvanbeek on gitter about tags especially string->string. things still seem sensible, especially as we are optimizing for simple and what works today.

Ex we currently have no aggregation that requires other data types, and even if we did, this could be mitigated by parsing consumer side. Further, anyOf values don't have wide support and when they do, they often make wrapper types which can be troublesome to serialize. Keeping the course of map<string,string> keeps focus on the job and away from things like this.

codefromthecrypt commented 7 years ago

also settling on timestamp/duration as we have today in normal span. It makes it easy to do duration, and doesn't prevent arithmetic to get finish timestamp. Turns out that async span is not easier or harder with this decision.

codefromthecrypt commented 7 years ago

OK the current work in progress proves the new format can not only work for instrumentation, but also as an input for dependency linking (before, we had an internal DependencyLinkSpan type).

So, right now, we have zipkin.simplespan.SimpleSpan

I'm inclined to make a new package called zipkin2 and rename SimpleSpan to zipkin2.Span etc. If we did that, we'd have the artifact "zipkin" depend on "zipkin2", and host converters in the former. The rationale is that it would be easier to explain that what "simplespan" means, and also encourage its use moving forward.

FYI: is exactly the same way we moved Brave to a new api (in pieces, doing small parts at a time instead of big bang). For example, we added apis to Brave 4 over the course of many minor releases, and we still release the old artifacts today.

Basically, the choices are zipkin.simplespan.SimpleSpan or zipkin2.Span for the single-host variant. Does anyone feel strongly about this? thumbsup means you like zipkin2.Span

cc @openzipkin/core @shakuzen @devinsba @llinder @reta @ImFlog @semyonslepov @sirtyro

devinsba commented 7 years ago

On the question about elasticsearch 2.3-, we just switched to 5.3 in AWS so we are cool with this

devinsba commented 7 years ago

Based on the amount of discussion lately on messaging, my gut feeling is that if we go the zipkin2.Span route that we make messaging concerns a first class citizen of the data model if needed. Specifically around the single producer multiple consumer case. Maybe there should be a Span.Kind that makes this relationship explicit and a defined practice around it.

Maybe I'm not following the current messaging story since I don't use it myself but I think if we go V2 we have the chance to make it first class is my point

codefromthecrypt commented 7 years ago

Messaging implies a boatload more work to elaborate than guessing we solve it with an enum choice.

I would suggest decoupling such an idea to a minor bump because it isnt required to rename a type to add another field or enum choice.

For example, this type now has direction where formerly we didnt. Ex localEndpoint and remoteEndpoint even if type is null can tell you the direction of communication.

What I mean to say is I don't think it is required to change the format incompatibly to handle messaging. On the other hand to formalize messaging still requires practice and whats out there now doesnt.

Are you flexible on this point? If not then we will need another set of converters (just for packaging) when we do decide it is ok to increment our major version number. This may lead to it never happening as has happened prior issues.

On 27 Jul 2017 12:35 am, "Brian Devins" notifications@github.com wrote:

Based on the amount of discussion lately on messaging, my gut feeling is that if we go the zipkin2.Span route that we make messaging concerns a first class citizen of the data model if needed. Specifically around the single producer multiple consumer case. Maybe there should be a Span.Kind that makes this relationship explicit and a defined practice around it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin/issues/1499#issuecomment-318110087, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD61wSCoudCPnySgLv7Jm3vsrQpxU8xks5sR2rrgaJpZM4Ln26F .

codefromthecrypt commented 7 years ago

@devinsba ps if a gun was to my head I would choose producer and consumer as the messaging kinds :) that's because it is single sided and knowing which side it is helps with dependency linking. We can alias this to client/server on ambiguity (ex like discussed before one side thinks messaging other rpc)

Even if messaging would be more work than this to complete, adding producer/consumer seem harmless and can clarify intent a bit better cc @Imflog @jcarres-mdsol @fedj

So counter proposal is :) add messaging enums, if nothing more to better express intent. Test them on the existing WIP branch for known confusion cases (ex pubsub where one side is rpc other is messaging). If that works, "rough in" messaging with these enums but save most work (UI etc) for later PR.

codefromthecrypt commented 7 years ago

PS technically the new "shared" flag for the receiver (server or consumer) helps with messaging as you can tell instrumentation error (more than one span with shared set for same span ID). This is important for SPMC but not well discussed in this thread.

codefromthecrypt commented 7 years ago

so here's next thing to vote while I experiment. thumbsup means you are cool with adding PRODUCER/CONSUMER to the kind enum in zipkin 2's (single host) span type. if frownie face, reply back why por favor

I offer labor to document what this means etc, as it is less than writing converters and then documenting them later :)

cc @markpollack @marcingrzejszczak who also have asked for this

ghost commented 7 years ago

CC'ing @garyd203 @ilutz @jonathan-lo

codefromthecrypt commented 7 years ago

here's the first piece, which adds an internal copy of span2 (currently without messaging types which will go in later) #1669

codefromthecrypt commented 7 years ago

1670: conversion utility between the original and new model from Span2 (splitting as necessary)

codefromthecrypt commented 7 years ago

1671: json codec for Span2

codefromthecrypt commented 7 years ago

1673 Drops internal DependencyLinkSpan for Span2

codefromthecrypt commented 7 years ago

1651 Elasticsearch uses Span2 internally

ghost commented 7 years ago

CC'ing @sigerber

openzipkin / zipkin

Simplified span2 format #1499

Scope

Format

Impact on tracers

Impact on servers

FAQ

Why just json?

Why not special-case the tracer's endpoint?

Why not change the query api, too?

Why not just remove support for the old model?

What about async spans?

1670: conversion utility between the original and new model from Span2 (splitting as necessary)

1671: json codec for Span2

1673 Drops internal DependencyLinkSpan for Span2

1651 Elasticsearch uses Span2 internally