[dev guide] Add docs on optional schema arguments #12371

fearful-symmetry · 2019-05-30T17:13:43Z

This is a response to #7335, as it adds a great deal of functionality to the schema library and none of it seems to be documented.

This PR expands the schema.Apply example to the beats developer guide with documented usage of the various optional arguments now available in schema.Apply

There's a few undressed things here:

should this be a separate example, to emphasize that this functionality is optional?
I would like to add a code snippet for NotFoundKeys but I'm still trying to figure it out.

kaiyan-sheng

Thanks for adding this! Very helpful!! We also have IgnoreAllErrors option as well. It is aded by #12089

dedemorton

A few changes. Thank you for adding this!

docs/devguide/create-metricset.asciidoc

dedemorton · 2019-05-30T17:36:23Z

docs/devguide/create-metricset.asciidoc

 }
 ----
+<1> `Required` will explicitly mark a field as non-optional.
+<2> If a field has no Schema Option set, it is equivalent to `Required`.


Not sure why "Schema Option" is capitalized here. Generally we only capitalize proper nouns. I would change this to say, "If no schema option is set, the field is required."

More than likely, I was looking at the code, which is capitalized, and my brain just translated it to words that way, hah.

dedemorton · 2019-05-30T17:39:24Z

docs/devguide/create-metricset.asciidoc

 }
 ----
+<1> `Required` will explicitly mark a field as non-optional.
+<2> If a field has no Schema Option set, it is equivalent to `Required`.
+<3> `Optional` You can also explicitly mark a field as not required.


Suggested change

<3> `Optional` You can also explicitly mark a field as not required.

<3> Marks the field as optional.

I understand why you are being precise here, but I think the content is harder to read when you swap between using "non-optional" and "not required." Except in situations where it's important to know that a setting is explicit (like you do later on where you say "field explicitly marked as required"), I would simply refer to a fields as required or optional. I'll suggest edits with this notion in mind. I hope I'm understanding things correctly.

Yah, I was struggling to word that in a way that didn't seem repetitive.

docs/devguide/create-metricset.asciidoc

dedemorton · 2019-05-30T17:42:48Z

docs/devguide/create-metricset.asciidoc

+<2> If a field has no Schema Option set, it is equivalent to `Required`.
+<3> `Optional` You can also explicitly mark a field as not required.
+<4> By default, `Apply` will fail and return an error if any field not marked as optional is missing. Using the optional second argument, you can specify how `Apply` handles different fields of the  schema. The possible values are:
+- `AllRequired` is the default behavior. Return an error if any field not marked as optional is missing.


Suggested change

- `AllRequired` is the default behavior. Return an error if any field not marked as optional is missing.

- `AllRequired` is the default behavior. Returns an error if any required field is missing.

If you want to disambiguate further, you could add "...any required field is missing, including fields that are required because no schema option is set."

Yah, that's what I was trying to get across. Thanks!

dedemorton · 2019-05-30T17:46:20Z

docs/devguide/create-metricset.asciidoc

+<4> By default, `Apply` will fail and return an error if any field not marked as optional is missing. Using the optional second argument, you can specify how `Apply` handles different fields of the  schema. The possible values are:
+- `AllRequired` is the default behavior. Return an error if any field not marked as optional is missing.
+- `FailOnRequired`  will fail if a field explicitly marked as `required` is missing.
+- `NotFoundKeys(cb func([]string))` will pass a callback function that will be called with the list of missing keys, for more fine-grained error handling.


This sentence was a little hard to parse on first reading. Would it be accurate to say "...will pass a callback function that returns a list of missing keys for more fine-grained error handling."

codecov · 2019-05-31T00:21:00Z

Codecov Report

Merging #12371 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12371      +/-   ##
==========================================
- Coverage   57.96%   57.92%   -0.04%     
==========================================
  Files         933      930       -3     
  Lines       57453    57339     -114     
==========================================
- Hits        33302    33215      -87     
+ Misses      21085    21065      -20     
+ Partials     3066     3059       -7

Impacted Files	Coverage Δ
libbeat/publisher/queue/memqueue/ringbuf.go	`67.88% <0%> (-5.51%)`	⬇️
auditbeat/module/file_integrity/action.go	`63.15% <0%> (-5.27%)`	⬇️
heartbeat/scheduler/scheduler.go	`71.12% <0%> (-3.21%)`	⬇️
libbeat/publisher/queue/memqueue/eventloop.go	`76.97% <0%> (-0.66%)`	⬇️
...beat/module/file_integrity/eventreader_fsevents.go
...ditbeat/module/file_integrity/fileorigin_darwin.go
auditbeat/module/file_integrity/fileinfo_bsd.go
libbeat/publisher/queue/spool/inbroker.go	`68.35% <0%> (+1.17%)`	⬆️
...ditbeat/module/file_integrity/monitor/recursive.go	`81.9% <0%> (+1.9%)`	⬆️
...beat/module/file_integrity/eventreader_fsnotify.go	`78.82% <0%> (+3.52%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf2f311...cb799a6. Read the comment docs.

fearful-symmetry · 2019-05-31T00:22:44Z

codecov, what are you doing

Anyways, thanks @dedemorton ! I struggle with how to grammatically format lists, hah.

jsoriano

Thanks for this! We could also add some godocs to the package, but this can be done in a separate PR.

jsoriano · 2019-05-31T08:43:47Z

docs/devguide/create-metricset.asciidoc

 }
 ----
+<1> Marks a field as required.
+<2> If a field has no schema option set, it is equivalent to `Required`.
+<3> Marks the field as optional.


@odacremolbap added a new option to ignore all errors of an specific field (#12089), it may worth to mention it here too.

IgnoreAllErrors option, it is aded by #12089

fearful-symmetry · 2019-05-31T14:06:42Z

Yep, @jsoriano I added a mention about the IgnoreAllErrors option after @kaiyan-sheng mentioned it.

fearful-symmetry · 2019-05-31T14:29:34Z

Also, do you have an idea of where we should add godocs for this? Just to Apply ?

jsoriano · 2019-05-31T15:17:56Z

Yep, @jsoriano I added a mention about the IgnoreAllErrors option after @kaiyan-sheng mentioned it.

Oh, but take into account that this is an option for the fields (like it could be in "test_string": c.Str("testString", s.IgnoreAllErrors), not for schema.Apply. From what I see here it looks like it is an option for schema.Apply.

Also, do you have an idea of where we should add godocs for this? Just to Apply ?

It could be added to the Apply godocs, or we could add some package-level godocs.
It could be also nice to document ApplyTo method, that is like a lower-ish level function than Apply. Feel free to create an issue for these pending godocs (I should have added them after #7335).

fearful-symmetry · 2019-05-31T15:52:33Z

Okay, having IgnoreAllErrors as a field option makes way more sense, hah.

I also added a doc example for ApplyTo

fearful-symmetry · 2019-06-03T15:23:21Z

jenkins, test this

docs/devguide/create-metricset.asciidoc

jsoriano

Thanks!

* add docs on optional schema arguments

add docs on optional schema arguments

65d2cc1

fearful-symmetry added docs Team:Integrations Label for the Integrations team labels May 30, 2019

fearful-symmetry requested review from dedemorton and a team May 30, 2019 17:13

fearful-symmetry self-assigned this May 30, 2019

remove extra newlines

ca869dd

kaiyan-sheng reviewed May 30, 2019

View reviewed changes

dedemorton suggested changes May 30, 2019

View reviewed changes

clean up grammar

cb799a6

fearful-symmetry requested review from dedemorton and a team May 31, 2019 00:21

jsoriano reviewed May 31, 2019

View reviewed changes

put IgnoreAllErrors in the right place, add ApplyTo docs

aabbe56

fearful-symmetry requested review from kaiyan-sheng and jsoriano May 31, 2019 15:51

example syntax

01eccba

dedemorton approved these changes May 31, 2019

View reviewed changes

andresrc added the [zube]: In Review label Jun 3, 2019

jsoriano reviewed Jun 3, 2019

View reviewed changes

docs/devguide/create-metricset.asciidoc Outdated Show resolved Hide resolved

docs/devguide/create-metricset.asciidoc Outdated Show resolved Hide resolved

fix typos, error handling in example

ba5720c

jsoriano approved these changes Jun 5, 2019

View reviewed changes

fearful-symmetry merged commit 8b237b5 into elastic:master Jun 5, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Jun 5, 2019

andresrc removed the [zube]: Done label Jun 12, 2019

andrewvc pushed a commit to andrewvc/beats that referenced this pull request Jun 12, 2019

[dev guide] Add docs on optional schema arguments (elastic#12371)

26c87ba

* add docs on optional schema arguments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dev guide] Add docs on optional schema arguments #12371

[dev guide] Add docs on optional schema arguments #12371

fearful-symmetry commented May 30, 2019

kaiyan-sheng left a comment

dedemorton left a comment

dedemorton May 30, 2019

fearful-symmetry May 31, 2019

dedemorton May 30, 2019

dedemorton May 30, 2019

fearful-symmetry May 31, 2019

dedemorton May 30, 2019

dedemorton May 30, 2019

fearful-symmetry May 31, 2019

dedemorton May 30, 2019

codecov bot commented May 31, 2019

fearful-symmetry commented May 31, 2019

jsoriano left a comment

jsoriano May 31, 2019

kaiyan-sheng May 31, 2019

fearful-symmetry commented May 31, 2019

fearful-symmetry commented May 31, 2019

jsoriano commented May 31, 2019

fearful-symmetry commented May 31, 2019

fearful-symmetry commented Jun 3, 2019

jsoriano left a comment

	<3> `Optional` You can also explicitly mark a field as not required.
	<3> Marks the field as optional.

	- `AllRequired` is the default behavior. Return an error if any field not marked as optional is missing.
	- `AllRequired` is the default behavior. Returns an error if any required field is missing.

[dev guide] Add docs on optional schema arguments #12371

[dev guide] Add docs on optional schema arguments #12371

Conversation

fearful-symmetry commented May 30, 2019

kaiyan-sheng left a comment

Choose a reason for hiding this comment

dedemorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 31, 2019

Codecov Report

fearful-symmetry commented May 31, 2019

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fearful-symmetry commented May 31, 2019

fearful-symmetry commented May 31, 2019

jsoriano commented May 31, 2019

fearful-symmetry commented May 31, 2019

fearful-symmetry commented Jun 3, 2019

jsoriano left a comment

Choose a reason for hiding this comment