search

openzipkin / zipkin-api

Zipkin's language independent model and HTTP Api Definitions
https://zipkin.io/zipkin-api/
Apache License 2.0
59 stars 32 forks source link

Adds /api/v2/traceMany?traceIds=4d1e00c0db9010db,86154a4ba6e91385 #82

Closed codefromthecrypt closed 5 years ago

codefromthecrypt commented 5 years ago

Please make comments on the RATIONALE.md

This feature is backed by https://github.com/openzipkin/zipkin/pull/2803

cc @openzipkin/core

codefromthecrypt commented 5 years ago

@openzipkin/core FYI I will close this PR, any associated issues as won't fix, and move the branch to attic if still no feedback by next week.

This isn't passive aggressive, I just wanted to help get code folks worked on landed, and prefer to land that. Setting up another half year of merge conflicts is an anti-goal though.

jcchavezs commented 5 years ago

I like it, great work! I am not sure it is worth to describe cases like:

  1. What if one or more of the traceId do not exist? What is it going to be response? I guess according to https://github.com/openzipkin/zipkin-api/pull/82/files#diff-a683e0894de17a57f5462176d4374f48R180 then just the existing ones will come but wouldn't it be good to let the user know which ones are missing? I am just thinking loud
  2. What if both traceIds are the same (this one is less likely)
codefromthecrypt commented 5 years ago

@jcchavezs

What if one or more of the traceId do not exist? What is it going to be response? I guess according to https://github.com/openzipkin/zipkin-api/pull/82/files#diff-a683e0894de17a57f5462176d4374f48R180 then just the existing ones will come but wouldn't it be good to let the user know which ones are missing?

I don't really advocate for complicating the model, since the user can just look to see if their trace IDs are present. By using the same response as the other trace query, we get some consistency goals. I'll put in a rationale part about why not indexed response. thx.

What if both traceIds are the same (this one is less likely)

I think this is a client error, and better to fix the caller. Will add a note

codefromthecrypt commented 5 years ago

Thanks for the quick reviews folks. I'll work in the feedback shortly

codefromthecrypt commented 5 years ago

ok feedback so far addressed IMHO

codefromthecrypt commented 5 years ago

Here's the implementation. I'll merge both Monday unless feedback says no https://github.com/openzipkin/zipkin/pull/2811