Skip to content

Stop using span pools for Mixer tracing.#1893

Merged
istio-merge-robot merged 1 commit intoistio:masterfrom
douglas-reid:no-pool-spans
Nov 28, 2017
Merged

Stop using span pools for Mixer tracing.#1893
istio-merge-robot merged 1 commit intoistio:masterfrom
douglas-reid:no-pool-spans

Conversation

@douglas-reid
Copy link
Copy Markdown
Contributor

@douglas-reid douglas-reid commented Nov 28, 2017

What this PR does / why we need it:

Turns span pooling off. Observing race conditions in post-submits related to spans. This will eliminate a potential source of those race conditions.

Note: I have not been able to locally repro the span issues, but based on Mixer architecture and warnings surrounding PoolSpans, I believe this to be a likely source of the race.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

@istio-testing
Copy link
Copy Markdown
Collaborator

@douglas-reid: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mandarjog
Copy link
Copy Markdown
Contributor

We have been unable to reproduce the error, however reusing spans can cause an NPE like this, so going ahead with this change.

/lgtm

@sebastienvas
Copy link
Copy Markdown
Contributor

/lgtm

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandarjog, sebastienvas

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 28, 2017

Codecov Report

Merging #1893 into master will decrease coverage by 0.52%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1893      +/-   ##
==========================================
- Coverage   81.67%   81.15%   -0.53%     
==========================================
  Files         201      186      -15     
  Lines       20447    18883    -1564     
==========================================
- Hits        16701    15324    -1377     
+ Misses       3275     3128     -147     
+ Partials      471      431      -40
Flag Coverage Δ
#broker 44.44% <ø> (-1.07%) ⬇️
#mixer 82.42% <ø> (-0.78%) ⬇️
#pilot 80.36% <ø> (-0.05%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
mixer/pkg/tracing/tracing.go 78.57% <ø> (ø) ⬆️
mixer/adapter/prometheus/server.go 89.83% <0%> (-5.09%) ⬇️
pilot/platform/consul/monitor.go 76.19% <0%> (-3.58%) ⬇️
pilot/platform/kube/inject/inject.go
mixer/pkg/runtime/env.go
broker/pkg/version/version.go
mixer/pkg/runtime/logger.go
mixer/pkg/runtime/handler.go
mixer/pkg/runtime/context.go
pilot/platform/kube/inject/initializer.go
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e36ed7b...9456c5c. Read the comment docs.

@douglas-reid
Copy link
Copy Markdown
Contributor Author

/retest

@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 istio-merge-robot merged commit 9d3e422 into istio:master Nov 28, 2017
kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
…filter. (istio#1893)

* Using dynamicMetadata to pass data between filters instead of headers

* Lint

* Populate authn result to dynamic data only.

* Integration test for authn

* Clean up and verify all tests

* Remove unused test configs

* Address reviews

* Lint
@douglas-reid douglas-reid deleted the no-pool-spans branch October 16, 2018 16:01
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.

6 participants