Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for grpc tracing header. #503

Merged
merged 3 commits into from
Jan 28, 2019

Conversation

bogdandrutu
Copy link
Contributor

@qiwzhang
Copy link
Contributor

@bogdandrutu could you fix the code format

Checking coding styles.
Files not formatted: src/api_manager/cloud_trace/cloud_trace.cc src/api_manager/cloud_trace/cloud_trace.h src/api_manager/cloud_trace/cloud_trace_test.cc src/api_manager/context/request_context.cc
Code style check failed.

You can run script/robot-style to check it. We are using clang-3.8.0

tc[kTraceIdFieldIdPos] = 0;
uint64_t tid_hi, tid_lo;
sscanf(trace_->trace_id().c_str(), "%016lx%016lx", &tid_hi, &tid_lo);
tid_hi = __builtin_bswap64(tid_hi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we assume always running in big endian. probably it is better not to do that.

Beside, I don't like to use builtin functions, not sure if it will break when you upgrade compiler.

private:
std::unique_ptr<google::devtools::cloudtrace::v1::Trace> trace_;
google::devtools::cloudtrace::v1::TraceSpan *root_span_;
std::string options_;
std::string original_trace_context_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Thanks.

} else {
// Set grpc trace context header to backend.
Status status =
request()->AddHeaderToBackend(kGRpcTraceContextHeader, trace_context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine two AddHeaderToBackend() calls into one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@bogdandrutu
Copy link
Contributor Author

@qiwzhang @bochunz I am blocked on this because the work to setup bazel (version from 1.5y ago) + the clang will take way too much on the google machine. I would suggest to offer a docker builder image that I can use and avoid polluting my workstation.

@bogdandrutu
Copy link
Contributor Author

bogdandrutu commented Jan 27, 2019

Made progress on being able to build and run clang-format in a docker image, I cannot run the nginx tests (I will spend some time on that).

I have some questions:

  1. Do you mind taking a dependency on absl?
  2. Can you change your own trace_client implementation with OpenCensus? https://github.com/census-instrumentation/opencensus-cpp

@qiwzhang
Copy link
Contributor

@bogdandrutu

  • we dont mind you use absl.
  • we may not have resource to change to use OpenCensus. But if there is business need, we may change priority.

@qiwzhang qiwzhang merged commit e9a1914 into cloudendpoints:master Jan 28, 2019
@eli-jordan
Copy link

eli-jordan commented May 27, 2019

@qiwzhang @bogdandrutu
I just hit this issue, and noticed the change in this PR behaves in a bit of a strange way. When running ESP with a gRPC backend:

  • If invoked directly over gRPC the grpc-trace-bin header is forwarded to the backend
  • If invoked over HTTP (i.e. using the transcoding feature) the x-cloud-trace-context header is forwarded to the backend.

If the backend is gRPC, I would expect the grpc-trace-bin to always be sent.

@qiwzhang
Copy link
Contributor

@bogdandrutu this is the feature: auto detect backend protocol use grpc-trace-bin or x-cloud-trace-context properly. I remember you were about to finish this task. What is your plan?

@qiwzhang
Copy link
Contributor

I used a separate issue to track it: #627

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants