Skip to content

TSDB: Add dimensions and timestamp to parse errors#84962

Merged
nik9000 merged 7 commits intoelastic:masterfrom
nik9000:tsdb_more_description
Mar 17, 2022
Merged

TSDB: Add dimensions and timestamp to parse errors#84962
nik9000 merged 7 commits intoelastic:masterfrom
nik9000:tsdb_more_description

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented Mar 14, 2022

This adds a list of dimensions and @timestamp to the parse errors we
make in tsdb if the _tsid or @timestamp are built.

@nik9000 nik9000 added the :StorageEngine/TSDB You know, for Metrics label Mar 14, 2022
@nik9000 nik9000 requested review from imotov and romseygeek March 14, 2022 21:28
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 14, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

This adds a list of dimensions and `@timestamp` to the parse errors we
make in tsdb if the `_tsid` or `@timestamp` are built.
Copy link
Copy Markdown
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

LGTM!

IndexableField timestampField = context.doc().getField(DataStreamTimestampFieldMapper.DEFAULT_PATH);
if (timestampField != null) {
String timestamp = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.formatMillis(timestampField.numericValue().longValue());
description.append(" at ").append(timestamp);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: we usually output values in error messages enclosed inside []. Dimensions above are a map, so toString() will automatically enclose them in {}. But maybe we can add it here so that users receive a message like this:

failed to parse field [foo] of type [long] in a time series document at [2021-04-28T00:01:00.000Z]. Preview of field's value: 'true'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Makes sense to me.

@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Mar 15, 2022

This relates to #84957

IndexableField tsidField = context.doc().getField(TimeSeriesIdFieldMapper.NAME);
if (tsidField != null) {
String tsid = TimeSeriesIdFieldMapper.decodeTsid(tsidField.binaryValue()).toString();
description.append(" with dimensions ").append(tsid);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@henningandersen and I talked about limiting this to maybe 1k characters or something. I hate that it'd make debugging hard but it's nice defense against a huge bulk response.

Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@nik9000 nik9000 requested review from csoulios and romseygeek March 16, 2022 16:51
@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Mar 16, 2022

@elasticmachine test this please

@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Mar 16, 2022

@elasticmachine test this please

@nik9000 nik9000 merged commit 22824e4 into elastic:master Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants