Skip to content

Add trace span for metadata fetching#778

Merged
qiwzhang merged 4 commits intocloudendpoints:masterfrom
qiwzhang:add_trace
Mar 27, 2020
Merged

Add trace span for metadata fetching#778
qiwzhang merged 4 commits intocloudendpoints:masterfrom
qiwzhang:add_trace

Conversation

@qiwzhang
Copy link
Copy Markdown
Contributor

Signed-off-by: Wayne Zhang qiwzhang@google.com

Also Not to create ServiceControlCheck and Quota span if it is not needed.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang requested review from JLXIA and TAOXUY March 26, 2020 19:36
@nareddyt nareddyt self-requested a review March 26, 2020 21:30
return;
} else if (!context->service_context()->service_control() ||
context->method()->skip_service_control()) {
TRACE(trace_span) << "Service control check is not needed";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would be careful with this. This changes behavior that is publicly documented: https://cloud.google.com/endpoints/docs/openapi/tracing#spans_created_by_esp

At a minimum, ESP creates 4 spans per trace

We'll have to update this documentation, not sure if it will confuse pre-existing users to see spans suddenly disappear.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On second though, less change, the better. So I will keep the old spans as they are.

}
continuation(Status::OK);
});
FetchMetadata(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does FetchMetadata do retries? I assume we won't create multiple traces if that occurs. Maybe that is OK, just curious.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It will do retry. But all retries will be in the same span,


void QuotaControl(std::shared_ptr<context::RequestContext> context,
std::function<void(Status status)> continuation) {
std::shared_ptr<cloud_trace::CloudTraceSpan> trace_span(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment about publicly documented behavior changing

Comment thread src/nginx/t/cloud_trace.t
@qiwzhang
Copy link
Copy Markdown
Contributor Author

Hold on. I will need trigger two more traces, Auth, and AuthFetch in cloud_trace.t

@qiwzhang
Copy link
Copy Markdown
Contributor Author

PTAL

Comment thread src/nginx/t/cloud_trace.t
@qiwzhang qiwzhang merged commit eedb380 into cloudendpoints:master Mar 27, 2020
@qiwzhang qiwzhang deleted the add_trace branch March 27, 2020 01:10
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