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.

Sign up for GitHub

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

Jump to bottom

Configurable ES doc count #2453

Merged
merged 16 commits into from
Sep 5, 2020
Merged

Configurable ES doc count #2453

merged 16 commits into from
Sep 5, 2020

Conversation

albertteoh
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Add deprecation note for --es.max-num-spans. (Will create another issue to decommission this flag for a future release).
  • Add new flag --es.max-doc-count, that configures query (and aggregation) doc sizes, defaulting to 10_000. Name based on existing defaultDocCount variable for consistency.
  • Fix tests which were incorrectly checking for the document size at the query level.
  • Add test to assert on aggregation sizes applied.

@albertteoh albertteoh requested a review from a team as a code owner September 3, 2020 13:38
@albertteoh albertteoh requested a review from vprithvi September 3, 2020 13:38
@albertteoh albertteoh force-pushed the configurable-aggregation-size branch from 760464c to 02681ba Compare September 3, 2020 13:42
albertteoh and others added 7 commits September 3, 2020 23:44
Configurable limits
393772f 
Signed-off-by: albertteoh <albert.teoh@logz.io>
aggregationSize -> maxDocCount
e7d6988 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Refactor create dependency reader
fe99b9a 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Stronger assertions on aggregation sizes
b809361 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Add deprecation note
45e4e1e 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Remove stuttered full-stop
702bffa 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Better comments and readability
2a0d5d0 
Signed-off-by: albertteoh <albert.teoh@logz.io>
@pavolloffay
Copy link
Member

@albertteoh could you please fix the commit history, it seems that the PR contains other commits.

Comments
2347697 
Signed-off-by: albertteoh <albert.teoh@logz.io>
@albertteoh albertteoh force-pushed the configurable-aggregation-size branch from ac1ad85 to 2347697 Compare September 3, 2020 21:09
@albertteoh
Copy link
Contributor Author

@albertteoh could you please fix the commit history, it seems that the PR contains other commits.

Thanks for pointing that out @pavolloffay, I must have messed up the rebase. I've cleaned this up so commit history should only show my changes.

Revert spanstore import
11de7e3 
Signed-off-by: albertteoh <albert.teoh@logz.io>
pavolloffay
pavolloffay reviewed Sep 4, 2020
View reviewed changes
cmd/opentelemetry/app/internal/reader/es/esdependencyreader/dependency_store.go Outdated Show resolved Hide resolved
plugin/storage/es/factory.go Outdated Show resolved Hide resolved
pkg/es/config/config.go Outdated Show resolved Hide resolved
albertteoh added 6 commits September 4, 2020 20:46
One-liner create dep reader
febfbaf 
Signed-off-by: albertteoh <albert.teoh@logz.io>
MaxNumSpans -> MaxDocCount
d931a79 
Signed-off-by: albertteoh <albert.teoh@logz.io>
OTEL: MaxNumSpans -> MaxDocCount
4aaa2ac 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Fix integration test
3f38316 
Signed-off-by: albertteoh <albert.teoh@logz.io>
TestMaxNumSpans -> TestMaxDocCount
5706854 
Signed-off-by: albertteoh <albert.teoh@logz.io>
Remove unused struct member in test
9ebf9d3 
Signed-off-by: albertteoh <albert.teoh@logz.io>
@codecov
Copy link

codecov bot commented Sep 4, 2020 •
edited
Loading

Codecov Report

Merging #2453 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2453      +/-   ##
==========================================
+ Coverage   95.58%   95.59%   +0.01%     
==========================================
  Files         208      208              
  Lines       10679    10690      +11     
==========================================
+ Hits        10207    10219      +12     
  Misses        399      399              
+ Partials       73       72       -1
Impacted Files Coverage Δ
plugin/storage/es/dependencystore/storage.go 90.74% <100.00%> (+0.17%) ⬆️
plugin/storage/es/factory.go 100.00% <100.00%> (ø)
plugin/storage/es/options.go 100.00% <100.00%> (ø)
plugin/storage/es/spanstore/reader.go 100.00% <100.00%> (ø)
plugin/storage/es/spanstore/service_operation.go 100.00% <100.00%> (ø)
plugin/storage/integration/integration.go 80.38% <0.00%> (-0.48%) ⬇️
cmd/query/app/server.go 93.33% <0.00%> (+2.22%) ⬆️

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 4120220...5a9d1c9. Read the comment docs.

pavolloffay
pavolloffay approved these changes Sep 4, 2020
View reviewed changes
plugin/storage/es/options.go Outdated Show resolved Hide resolved
plugin/storage/es/options_test.go Show resolved Hide resolved
@pavolloffay
Copy link
Member

@albertteoh could you please increase the test coverage? https://codecov.io/gh/jaegertracing/jaeger/pull/2453/diff

@pavolloffay
Copy link
Member

One more thing @albertteoh we should add this change to changelog in braking changes since the limit is applied to new set of queries - we should enumerate what queries from the UI perspective might be affected.

Address review comments
5a9d1c9 
Signed-off-by: albertteoh <albert.teoh@logz.io>
@albertteoh albertteoh force-pushed the configurable-aggregation-size branch from c7af448 to 5a9d1c9 Compare September 4, 2020 21:46
albertteoh
albertteoh commented Sep 4, 2020
View reviewed changes
Copy link
Contributor Author

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

One more thing @albertteoh we should add this change to changelog in braking changes since the limit is applied to new set of queries - we should enumerate what queries from the UI perspective might be affected.

Good point, I've added a note in the changelog for v1.20 (unreleased). Please let me know if anything in that changelog entry requires modification.

Thanks for reviewing!

plugin/storage/es/options_test.go Show resolved Hide resolved
plugin/storage/es/options.go Outdated Show resolved Hide resolved
plugin/storage/es/options.go Outdated Show resolved Hide resolved
@pavolloffay pavolloffay merged commit 9dad32f into jaegertracing:master Sep 5, 2020
@albertteoh albertteoh deleted the configurable-aggregation-size branch September 5, 2020 19:18
@richvdh richvdh mentioned this pull request Dec 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees
No one assigned
Labels
storage/elasticsearch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ES: Configurable query result sizes
2 participants
@albertteoh @pavolloffay