Skip to content

Conversation

@immon
Copy link
Contributor

@immon immon commented Sep 24, 2018

  • change keyword to integer for few fields
  • split types

* keyword to integer type change for few fields
* split types
* keyword to integer type change for few fields
* split types
@ruflin ruflin requested a review from ycombinator September 24, 2018 20:19
@ruflin
Copy link
Contributor

ruflin commented Sep 24, 2018

PR will need a changelog entry

"field": "elasticsearch.slowlog.types",
"separator": ",",
"ignore_missing": true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is slightly off.

"elasticsearch.slowlog.types": "",
"elasticsearch.slowlog.types": [
""
],
Copy link
Contributor

@ycombinator ycombinator Sep 25, 2018

Choose a reason for hiding this comment

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

I wonder if we want to index an array of one element, that element being an empty string, over here OR if we want to index an empty array OR not even index this field if no types are present.

Now that Ingest processors support if conditions (elastic/elasticsearch#32398), perhaps we could define the split processor like so:

{
   "split":{
      "field":"elasticsearch.slowlog.types",
      "separator":",",
      "ignore_missing":true,
      "if":"ctx.elasticsearch.slowlog.types != ''"
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin any opinion on that?
IMHO types field should be stored since it appears in the log line.
Arrays and single values are internally stored the same way since any ES field can be an array. Indeed _source will look different since source document is different, but should we care?

Copy link
Contributor

Choose a reason for hiding this comment

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

The log entry that this is coming from looks like this: total_hits[19435], types[], stats[],. If I run the following command in DevTools what I get is "a": []. So I would argue types should be there but with an empty array and not entry.

POST test/_doc
{
  "a": []
}

@immon
Copy link
Contributor Author

immon commented Oct 3, 2018

Thanks for review @ycombinator
I removed split processor to get PR merged. I wanted to change data type for few fields but got stuck on the split processor (and things like ES 6.5 installed to run tests)

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM

@jsoriano
Copy link
Member

jsoriano commented Nov 7, 2018

This will need to be updated with master and make update.

@immon immon requested a review from a team as a code owner December 19, 2018 13:30
@immon
Copy link
Contributor Author

immon commented Dec 19, 2018

@jsoriano this slipped under my radar. I make updated the code + fixed ECS stuff in tests but CI is failing due to lack of workers.

@jsoriano
Copy link
Member

jsoriano commented Jan 9, 2019

@immon sorry for the late reply. Could you try to update again?

@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@immon immon requested review from a team as code owners January 11, 2019 16:20
@jsoriano
Copy link
Member

There seems to still be something wrong with the expected json files 🤔

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

It LGTM, I have a doubt on the type for the shard id.

@jsoriano
Copy link
Member

jenkins, test this again please

1 similar comment
@immon
Copy link
Contributor Author

immon commented Jan 17, 2019

jenkins, test this again please

@immon immon requested review from a team as code owners January 22, 2019 10:15
@cwurm cwurm removed the request for review from a team January 22, 2019 12:58
@ruflin
Copy link
Contributor

ruflin commented Jan 24, 2019

This seems to have quite a bit of conflicts with master. Let's review again when it's rebased.

@exekias exekias removed the request for review from a team November 22, 2019 10:57
@ycombinator
Copy link
Contributor

ycombinator commented Feb 28, 2020

Hi @immon, could you rebase this PR on master please? That should help towards resolving the conflicts. Thanks!

Alternatively, you could just close this PR and make a new one, whichever is easier!

@immon
Copy link
Contributor Author

immon commented Mar 15, 2020

I totally lost track on that effort. It will be easier to create new PR since many things has changed since version 6.5 and 2018. I'm closing this one.

@immon immon closed this Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants