Skip to content
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

Bugfix: Pass docvalue_fields for elasticsearch #404

Merged
merged 7 commits into from
Jun 13, 2024
Merged

Conversation

idastambuk
Copy link
Contributor

@idastambuk idastambuk commented Jun 5, 2024

What this PR does / why we need it:
We had a bug reported in #388 (comment) where logs queries to ElasticSearch v7.8.1 with OpenSearch plugin were returning No Data. The reason for this was that we're sending "fields" here to OpenSearch with the timestamp format we desire.
ElasticSearch api, however, expects docvalue_fields instead, so it returns no data. Older versions of Elastic expect fields as well.

There was also some refactoring, since without it, the time field was in some cases added twice in fields or docvalue_fields (e.g. docvalue_fields: ["@timestamp", {"field": "@timestamp", "format": "date_time"}]

  • Moved adding the timeField to AddTimeFieldWithStandardizedFormat, where the field, along with its formatting will be added according to the flavor and version. Still not sure if fielddata_fields is completely necessary, but since it's only for Elastic < 5 and logs queries, it might not be relevant in time if we decide to deprecate. The same goes for the new version of AddDocValueFields function
  • removed script_fields field since it doesn't seem to be used anywhere

Which issue(s) this PR fixes:

Fixes #405

Special notes for your reviewer:
To test with Elastic 7.8.1:

  • make a logs query/raw document query
  • there should be data shown

@idastambuk idastambuk marked this pull request as ready for review June 7, 2024 13:18
@idastambuk idastambuk requested a review from a team as a code owner June 7, 2024 13:18
@idastambuk idastambuk requested review from kevinwcyu and njvrzm and removed request for a team June 7, 2024 13:18
Copy link
Contributor

@kevinwcyu kevinwcyu left a comment

Choose a reason for hiding this comment

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

LGTM. Do you know why it worked in the frontend, but not the backend path? I didn't see any of these fields in the frontend code.

@idastambuk
Copy link
Contributor Author

LGTM. Do you know why it worked in the frontend, but not the backend path? I didn't see any of these fields in the frontend code.

I think it's cause the desired format wasn't being set in the query so OpenSearch returns actual data. This however didn't guarantee we will get the expected timestamp format, which caused bugs, so adding it was definitely an improvement.

@idastambuk idastambuk merged commit e8590cf into main Jun 13, 2024
4 checks passed
@idastambuk idastambuk deleted the timestamp-format branch June 13, 2024 08:09
@idastambuk idastambuk mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using ElasticSearch 7.8, logs queries return no data
3 participants