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

Set lookback in ES rollover to distant past #3169

Merged
merged 3 commits into from
Aug 5, 2021

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Jul 28, 2021

Resolves #3152

@albertteoh could you please review?

@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #3169 (d35426a) into master (3732678) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3169      +/-   ##
==========================================
+ Coverage   95.91%   95.94%   +0.02%     
==========================================
  Files         239      239              
  Lines       14649    14655       +6     
==========================================
+ Hits        14051    14061      +10     
+ Misses        520      518       -2     
+ Partials       78       76       -2     
Impacted Files Coverage Δ
plugin/storage/es/spanstore/reader.go 100.00% <100.00%> (ø)
plugin/storage/integration/integration.go 78.88% <0.00%> (-0.40%) ⬇️
cmd/query/app/static_handler.go 97.00% <0.00%> (+1.19%) ⬆️
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 3732678...d35426a. Read the comment docs.

// In rollover only read alias is used to query the data so looking far into the past should
// not affect performance.
if p.UseReadWriteAliases {
maxSpanAge = time.Hour * 24 * 365 * 100
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should extend TestNewSpanReader, maybe converting it to a table-driven test, to prevent a regression on this functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

@albertteoh PR updated

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
reader := NewSpanReader(test.params)
require.NotNil(t, reader)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you're missing the assertion on test.maxSpanAge here (I would rename it test.wantMaxSpanAge to make it clear it's the expected value)?

Copy link
Member Author

Choose a reason for hiding this comment

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

doh, fixed

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay pavolloffay merged commit 5110efd into jaegertracing:master Aug 5, 2021
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Traces in a rolled-over index are unable to be hotlinked via /trace/{id}
3 participants