Skip to content

Fix time series timestamp meta missing #80695

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

Merged
merged 11 commits into from
Nov 22, 2021

Conversation

weizijun
Copy link
Contributor

I find a time series timestamp meta missing case.

here is the reproduce steps:

  1. create a time_series index, and set the timestamp field some meta.
  2. index a doc with a new field that is not in mappings, it will call mappings marge.
  3. then the timestamp field meta is missing.

the reason that meta is missing is when a new field comes, MappingParser.parse will build a new mapping with new fields. And merge the new mappings with exist mapping.
the new mapping have no timestamp field, so it will auto add timestamp field, the timestamp is without user's meta info.
And merge method build a new timestamp field to override the user's timestamp field.
It cause the timestamp meta missing.

I fixed the case, by move timestamp logic from MappingParser.parse to create index logic.
And move the tests to a new IT test.
I add a test to test case, TimeSeriesModeIT.testAddTimeStampMeta will fail in the pre-commit.

@elasticsearchmachine elasticsearchmachine added v8.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Nov 15, 2021
@nik9000 nik9000 added the :StorageEngine/TSDB You know, for Metrics label Nov 15, 2021
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 15, 2021
@elasticmachine
Copy link
Collaborator

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

@nik9000 nik9000 added >bug and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Nov 15, 2021
@weizijun
Copy link
Contributor Author

@elasticmachine update branch

@nik9000
Copy link
Member

nik9000 commented Nov 16, 2021

For my own education I reproduced this:

curl -uelastic:password -HContent-Type:application/json -XPUT localhost:9200/foo?pretty -d'{
  "settings": {
    "index": {
      "mode": "time_series",
      "routing_path": "dim"
    }
  },
  "mappings": {
    "properties": {
      "@timestamp": {
        "type": "date",
        "meta": {
          "m": "words words words"
        }
      },
      "dim": {
        "type": "keyword",
        "time_series_dimension": true
      }
    }
  }
}'


curl -uelastic:password localhost:9200/foo/_mappings?pretty
# meta is present

curl -uelastic:password -HContent-Type:application/json -XPOST localhost:9200/foo/_doc?pretty -d'{
  "@timestamp": "2021-11-16T09:48:20Z",
  "dim": "a",
  "cat": "mouse"
}'

curl -uelastic:password localhost:9200/foo/_mappings?pretty
# meta is absent

assertThat(getMappingsResponse.getMappings().get(index).source().string(), equalTo(Strings.toString(expect)));
}

}
Copy link
Member

Choose a reason for hiding this comment

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

I'd tend to want to do these as yaml rest tests instead of ESIntegTestCase. The yaml tests are much more copy and paste-ish, we run them during rolling upgrade testing and during backwards compatibility testing.

I'm not sure that it's worth rewriting it now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine, I move the test from TimeSeriesModeTests to TimeSeriesModeIT, some tests are the same with yaml tests. Maybe later, we can remove these tests.

for (Map<String, Object> mapping : mappings) {
List<Map<String, Object>> mergedMappings = new ArrayList<>();
boolean isTimeSeriesMode = indexService.getIndexSettings().getMode() != null
&& indexService.getIndexSettings().getMode() == IndexMode.TIME_SERIES;
Copy link
Member

Choose a reason for hiding this comment

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

I think IndexSettings.getMode can't be null - it'd default to STANDARD.

I think I'd prefer to move this merge helping code to IndexMode too, if we can. If every behavior change in different index modes is a method on IndexMode then folks reading it can scan it for the kinds of behavior that it influences. Hard checks for a particular mode like this make that much harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine, I'm trying to move these code into IndexMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change the code, is it ok?

Copy link
Member

@nik9000 nik9000 left a comment

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
Member

nik9000 commented Nov 18, 2021

@elasticmachine, test this please

@nik9000
Copy link
Member

nik9000 commented Nov 18, 2021

@elasticmachine update branch

@nik9000
Copy link
Member

nik9000 commented Nov 18, 2021

@elasticmachine test this please

@nik9000
Copy link
Member

nik9000 commented Nov 18, 2021

@weizijun the errors in the build look related to your change.

@weizijun
Copy link
Contributor Author

@weizijun the errors in the build look related to your change.

ok, I will fixed the tests

@nik9000
Copy link
Member

nik9000 commented Nov 19, 2021

@elasticmachine, test this please

@nik9000 nik9000 requested a review from martijnvg November 19, 2021 12:30
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nik9000 nik9000 merged commit 2136af2 into elastic:master Nov 22, 2021
@nik9000
Copy link
Member

nik9000 commented Nov 22, 2021

Thanks for your patience on this one @weizijun !

@weizijun
Copy link
Contributor Author

Thank you, @nik9000 , @martijnvg !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/TSDB You know, for Metrics v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants