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 SendGetBodyAs flag on elasticsearch storage #3193

Merged

Conversation

NatMarchand
Copy link
Contributor

Signed-off-by: Nathanael Marchand nathanael.marchand@outlook.com

Which problem is this PR solving?

Resolves #3191

Short description of the changes

This PR configures the property SendGetBodyAs on the elastic search client to allow sending HTTP requests with a body with another verb than GET (which might cause networking issue on some WAF or load balancers).
In order to do that, I've created an option -es.send-get-body-as to specify the verb to use.

I open this PR early to gather feedback as Go is not my native development language ;)

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

looks reasonable. Please run make fmt to fix formatting issues.

plugin/storage/es/options.go Outdated Show resolved Hide resolved
@NatMarchand NatMarchand force-pushed the feature/es-sendgetbodyas branch 2 times, most recently from a3d81d7 to 9fcc0fd Compare August 6, 2021 17:26
@NatMarchand
Copy link
Contributor Author

I've formatted code, improved the description such as suggested. I'm not sure if I have to add tests before removing Draft ?

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

usually we have some minimum test to ensure that the value is parsed properly into the struct

plugin/storage/es/options.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #3193 (3ec791b) into master (d56d9c3) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3193      +/-   ##
==========================================
- Coverage   95.94%   95.92%   -0.02%     
==========================================
  Files         239      239              
  Lines       14655    14661       +6     
==========================================
+ Hits        14061    14064       +3     
- Misses        518      520       +2     
- Partials       76       77       +1     
Impacted Files Coverage Δ
plugin/storage/es/options.go 100.00% <100.00%> (ø)
cmd/query/app/static_handler.go 95.80% <0.00%> (-1.80%) ⬇️
plugin/storage/badger/spanstore/reader.go 95.50% <0.00%> (-0.71%) ⬇️
cmd/query/app/server.go 95.58% <0.00%> (+1.47%) ⬆️

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 d56d9c3...3ec791b. Read the comment docs.

Signed-off-by: Nathanael Marchand <nathanael.marchand@outlook.com>
@yurishkuro yurishkuro marked this pull request as ready for review August 8, 2021 22:04
@yurishkuro yurishkuro requested a review from a team as a code owner August 8, 2021 22:04
@yurishkuro yurishkuro changed the title Draft: Add SendGetBodyAs on elasticsearch Add SendGetBodyAs on elasticsearch Aug 8, 2021
@yurishkuro yurishkuro changed the title Add SendGetBodyAs on elasticsearch Add SendGetBodyAs flag on elasticsearch storage Aug 8, 2021
@yurishkuro yurishkuro merged commit 79c15a8 into jaegertracing:master Aug 8, 2021
@yurishkuro
Copy link
Member

Thanks!

@NatMarchand NatMarchand deleted the feature/es-sendgetbodyas branch August 9, 2021 07:12
@NatMarchand
Copy link
Contributor Author

Thank you for your quick feedback and approval ! Glad to have contributed :)

@jpkrohling jpkrohling added this to the v1.26.0 milestone Aug 23, 2021
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.

Unable to find traces with Elasticsearch deployed behind Google Cloud load balancer
3 participants