Shippingservice rust

[GaryPWhite]

Fixes #35

Changes

remove golang implementation of shippingservice, written in rust now.

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue Reimplement 'shippingservice' in Rust #35

[GaryPWhite]

@TommyCpp

[GaryPWhite]

I'm aware that genproto.sh was removed from most projects in #94 -- I'm not sure if that's the right move here, since generating the files automatically with docker is not possible -- the build context required is not permitted by the fact the dockerfile sits two directories above where the original proto files live -- and generating them appropriately with build.rs necessitates the installation of cargo and mounting of a target folder (potentially from a different OS) into the container's build context. To build with docker, one would have to have cargo installed, which may defeat the purpose of using docker. Happy to discuss in detail as needed.

[GaryPWhite]

tl;dr, it generates code in the same directory as the target executables -- and the filenames are pseudo-random. if we could generate protos to some directory instead, that would be great -- but as I understand that's functionally similar to what genproto is doing anyways.

[mic-max]

I'm aware that genproto.sh was removed from most projects in #94 -- I'm not sure if that's the right move here, since generating the files automatically with docker is not possible -- the build context required is not permitted by the fact the dockerfile sits two directories above where the original proto files live -- and generating them appropriately with build.rs necessitates the installation of cargo and mounting of a target folder (potentially from a different OS) into the container's build context. To build with docker, one would have to have cargo installed, which may defeat the purpose of using docker. Happy to discuss in detail as needed.

To generate files in the Dockerfile as other services currently do, the build context .can be set in the docker-compose.yml file, check how the other services are setup in that file for examples. I'm not familiar with Rust so don't know what build.rs is. Seems like a multi-stage Dockerfile would be best for the build. First stage to get all the tools needed and compile the proto and code. Then the second (which would be published) is just the executable essentially.

[mic-max]

LGTM overall, thanks for doing this!!

Did as thorough of a review as I could without knowing Rust, mostly just had some comments about the protocol buffers which I'd be happy to have addressed in a follow up PR

[julianocosta89]

Added some comments, but the main blocker for me is the Context Propagation.
When running this version I just got a single orphan span for the shipping service.

[GaryPWhite]

@mic-max Re: Docker and protos: I assumed that the dockerfile should work without the docker-compose -- but it seems like the genproto.sh removal/change prefers that we use docker-compose from the base to build dockerfiles -- almost all dockerfiles are copying ./pb/ and using context: ./ -- I hadn't considered it :) I'll switch it over now.

To generate files in the Dockerfile as other services currently do, the build context .can be set in the docker-compose.yml file, check how the other services are setup in that file for examples. I'm not familiar with Rust so don't know what build.rs is.

build.rs is what cargo (think maven, gradle, npm) would use pre-compilation, or to set parameters during compilation. As I was thinking about this over the long weekend I think there might be a (slightly gross) solution, so long as the pb directory doesn't go anywhere :)

Seems like a multi-stage Dockerfile would be best for the build. First stage to get all the tools needed and compile the proto and code. Then the second (which would be published) is just the executable essentially.

We are using a multi-stage dockerfile -- I think the main issue here is that editing the code and actually running it without the protos in a place was a brain block for me -- I can use a solution as stated above

[GaryPWhite]

noting that I've included a change in the next commit that updates loadgenerator's dockerfile to use the not-slim image for python. M1's apparently don't like the slim version! #178 describes the issue, thanks for being patient @julianocosta89 !

[GaryPWhite]

@TommyCpp I used your sample (thanks!) for context propagation, I think I'm still missing something sending events over.. would really appreciate a wise set of eyes on if I'm doing this right :)

[julianocosta89]

@GaryPWhite I've played around a bit with and got the Context Propagation working:

I'm adding the changes that I've done in the code review.

[julianocosta89]

Suggested changes on main .rs are for the Context Propagation.
Suggested changes on shipping_service.rs are to make the events to work.

[TommyCpp]

@TommyCpp I used your sample (thanks!) for context propagation, I think I'm still missing something sending events over.. would really appreciate a wise set of eyes on if I'm doing this right :)

Sorry didn't get a chance to take a look at it during the day. But I think @julianocosta89 is right on point 😃

parent_cx contains remote span so we want to use it as a parent of our local span

[julianocosta89]

@GaryPWhite it seems that some files that were deleted previously got back in your last commit.
Was that expected, or they just sneaked in 😅 ?

[GaryPWhite]

@julianocosta89 hey wait -- which files are we talking about here? I don't know which ones I put in 😅

[julianocosta89]

@julianocosta89 hey wait -- which files are we talking about here? I don't know which ones I put in 😅

src/shippingservice/proto/demo.proto
src/shippingservice/proto/grpc/health/v1/health.proto
src/shippingservice/src/health_service.rs

And I also saw that some items that you had resolved before were back.
Like the blank line at the end of the files:
src/shippingservice/src/shipping_service/tracking.rs
src/shippingservice/src/shipping_service/quote.rs

[julianocosta89]

@julianocosta89 hey wait -- which files are we talking about here? I don't know which ones I put in 😅

src/shippingservice/proto/demo.proto src/shippingservice/proto/grpc/health/v1/health.proto src/shippingservice/src/health_service.rs

And I also saw that some items that you had resolved before were back. Like the blank line at the end of the files: src/shippingservice/src/shipping_service/tracking.rs src/shippingservice/src/shipping_service/quote.rs

Looks like I'm seeing stuff 👀

[julianocosta89]

@GaryPWhite yeah...

The getQuote event is not showing up 😢

[julianocosta89]

LGTM 🚀 !

[julianocosta89]

Thank you very much for the contribution @GaryPWhite !

And thanks @TommyCpp for helping us out 😍 in the troubleshooting!

[TommyCpp]

Nice work and thanks for making this happens! @GaryPWhite

[GaryPWhite]

woohoo! Just need someone with permission to merge and we're officially in rust! Thanks for all the help everyone :)

[cartersocha]

woohoo! Just need someone with permission to merge and we're officially in rust! Thanks for all the help everyone :)

Could you update your branch? then I'll merge

[julianocosta89]

@GaryPWhite looks like there are some merge conflicts there.
Would you mind taking a look?

I think after that we are good to merge it!

[GaryPWhite]

agh -- sorry for the delay! Fixed the conflict, should be good to go now!