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

Do not truncate tags in Elasticsearch #1970

Merged
merged 3 commits into from
Dec 11, 2019
Merged

Do not truncate tags in Elasticsearch #1970

merged 3 commits into from
Dec 11, 2019

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Dec 9, 2019 •
edited
Loading

Resolves #1960

KV.AsString() truncates binary tag value if it is longer than 256 and adds ... at the end. This causes failures when reading and decoding the tag value.

Signed-off-by: Pavol Loffay ploffay@redhat.com

@pavolloffay
Do not truncate tags in Elasticserch
e0c379d 
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@codecov
Copy link

codecov bot commented Dec 9, 2019 •
edited
Loading

Codecov Report

Merging #1970 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1970      +/-   ##
==========================================
+ Coverage   96.99%   96.99%   +<.01%     
==========================================
  Files         203      203              
  Lines       10061    10062       +1     
==========================================
+ Hits         9759     9760       +1     
  Misses        264      264              
  Partials       38       38
Impacted Files Coverage Δ
cmd/collector/app/sanitizer/utf8_sanitizer.go 100% <100%> (ø) ⬆️
plugin/storage/es/spanstore/dbmodel/from_domain.go 100% <100%> (ø) ⬆️
model/keyvalue.go 100% <100%> (ø) ⬆️
cmd/collector/app/http_handler.go 100% <0%> (ø) ⬆️

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 c1bc28d...78b1e49. Read the comment docs.

yurishkuro
yurishkuro reviewed Dec 9, 2019
View reviewed changes
plugin/storage/es/spanstore/dbmodel/from_domain_test.go
Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues
Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues
Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues
Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues Bender Bending Rodrigues `
Copy link
Member

Choose a reason for hiding this comment

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

this was supposed to be "Bender is Great!" string repeated :-)

@yurishkuro
Copy link
Member

could you elaborate in the description why this solves the truncation problem? Does KV.AsString() truncate values?

@yurishkuro
Copy link
Member

Does KV.AsString() truncate values?

Should we add, instead, a function there that doesn't truncate?

@pavolloffay
Copy link
Member Author

Should we add, instead, a function there that doesn't truncate?

It is a good idea, I will rename the current func to AsStringLossy and add AsString which does not truncate.

@pavolloffay
Add AsString to model and rename original to Lossy
eac88a2 
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
yurishkuro
yurishkuro reviewed Dec 10, 2019
View reviewed changes
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.

in most instances calling Lossy version is not actually required, let's keep it just AsString

model/keyvalue.go Show resolved Hide resolved
cmd/collector/app/span_processor_test.go Outdated Show resolved Hide resolved
model/converter/json/from_domain.go Outdated Show resolved Hide resolved
model/keyvalue_test.go Outdated Show resolved Hide resolved
model/keyvalue_test.go Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
plugin/storage/badger/spanstore/writer.go Outdated Show resolved Hide resolved
plugin/storage/cassandra/spanstore/dbmodel/unique_tags.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
@pavolloffay
Review comments
78b1e49 
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay pavolloffay merged commit 071fc85 into jaegertracing:master Dec 11, 2019
@pavolloffay pavolloffay added this to the Release 1.16 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@black-adder black-adder Awaiting requested review from black-adder

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@objectiser objectiser Awaiting requested review from objectiser

@tiffon tiffon Awaiting requested review from tiffon

@vprithvi vprithvi Awaiting requested review from vprithvi

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

Successfully merging this pull request may close these issues.

HTTP Error: Converting JSONSpan to domain Span failed
2 participants
@pavolloffay @yurishkuro