-
Notifications
You must be signed in to change notification settings - Fork 11
Go gRPC, HTTP client and server interoperability tests implemented #3
Conversation
HTTP client and server interop test with the various propagators implemented. The HTTP request is a POST request with a blank body. The HTTP response is a JSON serialization of the protobuf EchoResponse definition containing the: * TraceID * SpanID * TraceOptions and the HTTP request's propagation is with: * B3 * Google * TraceContext
Thanks to a tip from @rakyll and @Ramonza, can now check for tags by: * The server serializing their encoding as bytes * The client looking up and decoding the tag.Map from the response bytes This also warranted a change to the protobuf definitions hence, hopefully the setup and Go interop test harness is complete.
if err != nil { | ||
t.Fatalf("Failed to decode tagBlob (% X) err: %v", res.TagsBlob, err) | ||
} | ||
tag.Delete(mustKey("method")).Mutate(inTagMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds like you found a bug. I don't think we should be sending a method tag - what is the point? would you mind opening an issue on opencensus-go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll file the bug in a second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Propagation: fh, | ||
} | ||
|
||
atRuntimeHandler.ServeHTTP(w, r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is pretty weird - why not just set up separate routes that 1 for each propagation format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was simpler and sufficient to automatically create these :)
integration/Makefile
Outdated
@@ -0,0 +1,4 @@ | |||
all: gen_proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a make test
to set everything up and run all the tests?
@@ -0,0 +1 @@ | |||
## Interop testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets add instructions on running here, which ideally should just be "run make test"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you!
envAddrKey string | ||
fallbackAddr string | ||
}{ | ||
{name: "GoClient-GoServer", envAddrKey: "GO_SERVER_ADDR", fallbackAddr: ":9800"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda weird that this is being configured in Go: i would expect the server/client combination to be specified at the top level in a shell script / makefile so other languages wouldn't need to be concerned with editing Go files
@odeke-em did you get contributor access to this repo yet? |
* Added and documented environment variables for running the respective servers * Added the test Makefile directive to run all the tests. Just waiting on the Java clients and servers, include their addresses and then their tests and we'll be golden.
@Ramonza unfortunately I haven't yet gotten contributor access, but I'll use my own fork in the meantime. |
Thank you for the review @Ramonza! I'll merge this in to ensure that the Java team can bootstrap using the Go servers and the proto definitions as well as setup. We can keep iterating on this. |
Go gRPC, HTTP client and server interoperability tests implemented
No description provided.