Skip to content

LogsDB host and timestamp mappings tests #114001

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

Conversation

salvatore-campagna
Copy link
Contributor

@salvatore-campagna salvatore-campagna commented Oct 3, 2024

Here we are testing mappings of host and timestamp fields as they are
used as default fields to sort on when using LogsDB. LogsDB uses a
host.name field mapped as a keyword and a @timestamp field, required
by data streams. Some mappings throw errors as a result of incompatibilities
when trying to merge object fields. Such errors are expected.

See also #113600.

@salvatore-campagna salvatore-campagna added >test Issues or PRs that are addressing/adding tests :StorageEngine/Logs You know, for Logs labels Oct 3, 2024
@salvatore-campagna salvatore-campagna self-assigned this Oct 3, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@salvatore-campagna
Copy link
Contributor Author

Adding such tests is done to capture issues observed by Kibana. See #113600.

composed_of: ["logsdb-mappings"]

- match: { error.type: "illegal_argument_exception" }
- match: { error.reason: "composable template [logsdb-index-template] template after composition with component templates [logsdb-mappings] is invalid" }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not that informative.. any chance we can catch this and complain about @timestamp?

Copy link
Contributor Author

@salvatore-campagna salvatore-campagna Oct 7, 2024

Choose a reason for hiding this comment

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

The is just the top level exception message. There is another one saying

The timestamp field [@timestamp] does not exist in the data stream

I prefer not to change this as part of this PR...this tests just captures whatever the status is now.

Copy link
Member

Choose a reason for hiding this comment

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

This PR is about adding tests for what current expected behavior is. We can consider improving the error message in a follow up PR. I suspect this error is data stream related and not logsdb related. There is data stream logic in place around @timestamp.

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.

I think we should move this new yaml file to logsdb project. I don't think these very specific tests need to be part of yaml spec rest tests.

LGTM otherwise.

composed_of: ["logsdb-mappings"]

- match: { error.type: "illegal_argument_exception" }
- match: { error.reason: "composable template [logsdb-index-template] template after composition with component templates [logsdb-mappings] is invalid" }
Copy link
Member

Choose a reason for hiding this comment

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

This PR is about adding tests for what current expected behavior is. We can consider improving the error message in a follow up PR. I suspect this error is data stream related and not logsdb related. There is data stream logic in place around @timestamp.

@salvatore-campagna
Copy link
Contributor Author

I think we should move this new yaml file to logsdb project. I don't think these very specific tests need to be part of yaml spec rest tests.

LGTM otherwise.

Ok I will move these tests there.

@salvatore-campagna salvatore-campagna requested a review from a team as a code owner October 8, 2024 13:53
@salvatore-campagna
Copy link
Contributor Author

@martijnvg I moved yaml rest tests to the losgdb xpack plugin.


import static org.elasticsearch.test.cluster.FeatureFlag.FAILURE_STORE_ENABLED;

public class LogsDBClientYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Why not reuse LogsdbTestSuiteIT class?

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 didn't see it was there...I had to merge main

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

@salvatore-campagna
Copy link
Contributor Author

@elasticmachine update branch

@martijnvg
Copy link
Member

I think this should also be back ported to 8.x?

@salvatore-campagna
Copy link
Contributor Author

I think this should also be back ported to 8.x?

Yes

@salvatore-campagna salvatore-campagna added auto-backport Automatically create backport pull requests when merged v8.16.0 labels Oct 9, 2024
@salvatore-campagna
Copy link
Contributor Author

@elasticmachine update branch

@salvatore-campagna salvatore-campagna merged commit c4815b3 into elastic:main Oct 9, 2024
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 114001

@salvatore-campagna
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Questions ?

Please refer to the Backport tool documentation

salvatore-campagna added a commit to salvatore-campagna/elasticsearch that referenced this pull request Oct 9, 2024
Here we are testing mappings of `host` and `timestamp` fields as they are
used as default fields to sort on when using LogsDB. LogsDB uses a
`host.name` field mapped as a `keyword` and a `@timestamp` field, required
by data streams. Some mappings throw errors as a result of incompatibilities
when trying to merge object fields. Such errors are expected.

(cherry picked from commit c4815b3)
matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 10, 2024
Here we are testing mappings of `host` and `timestamp` fields as they are
used as default fields to sort on when using LogsDB. LogsDB uses a
`host.name` field mapped as a `keyword` and a `@timestamp` field, required
by data streams. Some mappings throw errors as a result of incompatibilities
when trying to merge object fields. Such errors are expected.
salvatore-campagna added a commit that referenced this pull request Oct 11, 2024
Here we are testing mappings of `host` and `timestamp` fields as they are
used as default fields to sort on when using LogsDB. LogsDB uses a
`host.name` field mapped as a `keyword` and a `@timestamp` field, required
by data streams. Some mappings throw errors as a result of incompatibilities
when trying to merge object fields. Such errors are expected.

(cherry picked from commit c4815b3)
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Oct 13, 2024
Here we are testing mappings of `host` and `timestamp` fields as they are
used as default fields to sort on when using LogsDB. LogsDB uses a
`host.name` field mapped as a `keyword` and a `@timestamp` field, required
by data streams. Some mappings throw errors as a result of incompatibilities
when trying to merge object fields. Such errors are expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending :StorageEngine/Logs You know, for Logs Team:StorageEngine >test Issues or PRs that are addressing/adding tests v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants