-
Notifications
You must be signed in to change notification settings - Fork 212
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
truncate strings that are stored as keywords in ES (#159) #159
Conversation
babcb30
to
4961552
Compare
this should avoid schema errors for values that are too long closes elastic#156 closes elastic#159
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general issue, I'd say handle checking required fields as well, I can see that you have placeholders for some fields. but that's a separated issue.
def keyword_field(string): | ||
if not isinstance(string, compat.string_types) or len(string) <= KEYWORD_MAX_LENGTH: | ||
return string | ||
return string[:KEYWORD_MAX_LENGTH - 1] + u'…' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beniwohli , Adding '…' would add 3 characters to the length which might exceed 1024 limit.
A separate issue: In my opinion '...' is not necessary, I would prefer adding a property to say the name was truncated and let the ui decide about the "ellipsis", but even that I think at this stage is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The character I use there is the unicode ellipsis, so it's only one character :)
I'm not sure adding an additional field for every keyword field to indicate if it was truncated or not is feasible. Having to truncate a field should be an absolute edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, sorry my python is a bit rusty 😄 !
It doesn't have to be a field for every keyword field. But my point was that I would prefer not adding anything to the fields that might have a meaning in different contexts and keeping what the data was originally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve the changes, then you can decide about the 'ellipsis'!
return result | ||
|
||
def get_process_info(self): | ||
return { | ||
'pid': os.getpid(), | ||
'argv': sys.argv, | ||
'title': None, | ||
'title': None, # Note: if we implement this, the value needs to be wrapped with keyword_field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your note to your future self 😄 , I do that as well!
4961552
to
45d32c2
Compare
this should avoid schema errors for values that are too long closes elastic#156 closes elastic#159
this should avoid schema errors for values that are too long closes elastic#156 closes elastic#159
45d32c2
to
38f8033
Compare
this should avoid schema errors for values that are too long
closes #156
closes #159