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

Use correct context on ES search methods #1850

Merged

Conversation

rubenvp8510
Copy link
Contributor

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

Which problem is this PR solving?

  • The actual ES code uses background context on search methods, this prevent some features to work properly (in this case I detected that token propagation does not work correctly).

Short description of the changes

  • Now it uses the context from which is called.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #1850 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1850      +/-   ##
==========================================
+ Coverage   98.42%   98.44%   +0.02%     
==========================================
  Files         197      197              
  Lines        9633     9631       -2     
==========================================
  Hits         9481     9481              
+ Misses        116      114       -2     
  Partials       36       36
Impacted Files Coverage Δ
plugin/storage/es/spanstore/reader.go 100% <100%> (ø) ⬆️
...lugin/sampling/strategystore/adaptive/processor.go 100% <0%> (+0.79%) ⬆️

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 cf286a0...3cfbccb. Read the comment docs.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Is the writer propagating the correct context?

@pavolloffay pavolloffay merged commit 6b5c9e4 into jaegertracing:master Oct 10, 2019
radekg pushed a commit to Klarrio/jaeger that referenced this pull request Oct 28, 2019
* Use correct context on ES search methods

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

* Updte reader tests, now expect valueCtx

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: radekg <radek@gruchalski.com>
@yurishkuro yurishkuro added this to the Release 1.15 milestone Nov 7, 2019
backjo pushed a commit to backjo/jaeger that referenced this pull request Dec 19, 2019
* Use correct context on ES search methods

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

* Updte reader tests, now expect valueCtx

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Jonah Back <jonah@jonahback.com>
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