TSDB: improve document description on error#84903
TSDB: improve document description on error#84903elasticsearchmachine merged 6 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
I've added a bunch of reviewers. I think any one is probably ok. A point of conversation: we could use this infrastructure to improve the error messages generated when we fail to parse documents in standard mode with an auto-generated id. It'd be a little more complicated than this change, but not much frankly. |
When a document fails to index we'd previously return an error with the description: ``` failed to parse field [a] of type [integer] in document with id 'null'. Preview of field's value: 'not_an_int' ``` That `with id 'null'` thing is kind of unfortunate. The document *will* have an id. We just don't know it. So now it says: ``` "failed to parse field [a] of type [integer] in a time series document. Preview of field's value: 'not_an_int'" ``` It's be nice to put the `@timestamp` and the `_tsid` if we have it. But we mostly won't.
romseygeek
left a comment
There was a problem hiding this comment.
LGTM! I left a couple of nits, but no re-review required.
server/src/main/java/org/elasticsearch/index/mapper/SourceToParse.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/TsidExtractingIdFieldMapper.java
Show resolved
Hide resolved
…rse.java Co-authored-by: Alan Woodward <romseygeek@gmail.com>
Thanks! I've merged your suggestion and added the comment you mentioned. And I'll let the robot merge it once everything is checked. |
imotov
left a comment
There was a problem hiding this comment.
LGTM, but I think if we could at least extract the timestamp, it would have help to significantly reduce the scope of documents that you need to potentially check.
| * folks really want to to see is the dimensions and timestamp. And | ||
| * those too aren't available until after we've parsed the document. | ||
| */ | ||
| return "a time series document"; |
There was a problem hiding this comment.
I think specifying a timestamp here might be really helpful.
There was a problem hiding this comment.
👍
I was thinking we could include the tsid and the timestamp if we have it but I didn't want to make this even more complex. I'll open a follow up once this is in and happy.
When a document fails to index we'd previously return an error with the
description:
That
with id 'null'thing is kind of unfortunate. The document willhave an id. We just don't know it. So now it says:
It's be nice to put the
@timestampand the_tsidif we have it. Butwe mostly won't.