Skip to content

Create a child span for mixer check call#1610

Merged
istio-merge-robot merged 3 commits into
istio:release-0.8from
bianpengyuan:tracing
May 2, 2018
Merged

Create a child span for mixer check call#1610
istio-merge-robot merged 3 commits into
istio:release-0.8from
bianpengyuan:tracing

Conversation

@bianpengyuan
Copy link
Copy Markdown
Contributor

No description provided.

@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Apr 28, 2018
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

LGTM

@qiwzhang
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

Copy link
Copy Markdown
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

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

Envoy has an API for filters to create a child span from the current Envoy Span. Please see if that can be used.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@rshriram
Copy link
Copy Markdown
Member

@rshriram
Copy link
Copy Markdown
Member

/lgtm cancel

@bianpengyuan
Copy link
Copy Markdown
Contributor Author

@rshriram @qiwzhang Updated. Ptal.

@bianpengyuan
Copy link
Copy Markdown
Contributor Author

@kyessenov @douglas-reid @mandarjog Just FYI the new trace will look like this:
screenshot from 2018-05-01 11-57-21
and an individual check span:
screenshot from 2018-05-01 11-57-51
The report/check name problem will be resolved by istio/istio#5335. If we want to separate ingress out of the first hop, start_child_span needs to be true in ingress router config.

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented May 1, 2018 via email

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented May 1, 2018

/lgtm
/approve

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@mandarjog
Copy link
Copy Markdown
Contributor

@geeknoid this PR is ready to merge.
@hklai the merge bot is indefinitely spamming.

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented May 2, 2018

Is there an issue tracking that this is needed for 0.8?

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@bianpengyuan
Copy link
Copy Markdown
Contributor Author

/test proxy-presubmit-asan

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@kyessenov
Copy link
Copy Markdown
Contributor

@geeknoid the issue is istio/old_issues_repo#306 (and other instances, this was debugged for awhile). I think it's pretty important, since without the fix, we get incorrect traces (a regression from 0.7).

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented May 2, 2018

Once again, please DO NOT change the setting of a robot. A robot need right access to do its job.

wx20180502-093913 2x

@hklai
Copy link
Copy Markdown
Contributor

hklai commented May 2, 2018

It might have been changed in the attempt to stop the merge from merging to master, which is now locked due to code orange.

Let's just disable the merge bot also for istio/proxy for now (already done that for istio/istio).

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented May 2, 2018

Please contact @istio/test-infra-hackers for the help and behavior change of istio robots.

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit 178406d into istio:release-0.8 May 2, 2018
@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented May 2, 2018

Disabled merge robot

@douglas-reid
Copy link
Copy Markdown
Contributor

@bianpengyuan @kyessenov what are the chances we can get the updated proxy image with these changes in now as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.