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

Add user_agent.original.text and fix multi_fields support #575

Merged
merged 18 commits into from
Dec 6, 2019

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Oct 2, 2019

This PR has a dual purpose:

  • It's meant to finalize the work on Split user-agent original into text and keyword #555. Thanks for getting this started, @mbudge!
  • Since this is the first multi-fields in ECS since 1.0.0, we needed to fix
    support for them in the generator. After this PR, the multi-fields appear:
    • as a separate line in the csv.
    • as a simplified mention in the main docs, we will likely want to improve this.
    • as a proper definition in the Elasticsearch template samples.
      Caveat: there isn't full support for setting defaults per type on multi-fields.
      The only default being set is setting norms:false for text.

@webmat webmat self-assigned this Oct 2, 2019
@webmat webmat requested a review from ruflin October 2, 2019 20:51
@webmat
Copy link
Contributor Author

webmat commented Oct 2, 2019

Note: I don't think this will make 1.2.0, so no rush.

@@ -76,8 +77,15 @@ def render_field_details_row(field):
example = ''
if 'example' in field:
example = "example: `{}`".format(str(field['example']))

field_name_with_mf = field['flat_name']
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 we will need to add some more docs to this code or extract it to documented functions. Not required now).

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 Yes, I would like to have the field set pages be expanded this way eventually:

  • Title & description of field set at the top (like now)
  • More compact table with short definitions only, and field names are links to a more complete section lower in the page
  • Perhaps a "field reuse" section like now, or integrate field reuse directly in this summary table described above
  • One linkable section per field name, with more complete details. Using headings for those will allow for the linking, and will also allow for additional information that's currently difficult to capture in the table we have not (should it be an array or a single value?, multi-fields details, deprecation, is the field index:false, etc.)

if 'group' not in f.keys():
# If no group set, set parent group
f["group"] = group
# if "multi_fields" in field:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of commenting out, can this code be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for pointing out.

@webmat
Copy link
Contributor Author

webmat commented Dec 5, 2019

I'd like to merge this soon and follow up with another PR adding the text analyzer to other fields.

I've made minor adjustments since your review, @ruflin:

  • moved the mention of multi-fields from the left column to the center column (below type)
  • changed user_agent.original description to remove the word "keyword"
  • adjusted CSV line for multi-fields to the new CSV format
  • added changelog entries

It's important to review how it looks in the docs preview. Prior to today's adjustments it looked like this (no longer the case):

Screenshot 2019-12-05 16 47 56

The new format (see preview) is more complete and clearer, but I'm not a big fan. A page redesign is out of the question for this PR, but I'm very much open to suggestions on how we can make this look acceptable in the meantime.

cc @rw-access @randomuserid and 🎁 @neu5ron ;-)

@webmat
Copy link
Contributor Author

webmat commented Dec 6, 2019

Ok, merging this now, as I need to make progress. Please chime in here if this causes issues, and I'll address those in a follow-up PR

@webmat webmat merged commit 060c6eb into elastic:master Dec 6, 2019
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.

None yet

3 participants