Skip to content

Conversation

@sudo-suhas
Copy link
Contributor

The keyed parameter is supported by the following aggregations:

However, in the reference guide, the keyed parameter is documented only for Range Aggregation.
This PR adds the documentations for the rest.

Closes #23731.

Add 'keyed' parameter documentation for following:
 - Date Histogram Aggregation
 - Date Range Aggregation
 - Geo Distance Aggregation
 - Histogram Aggregation
 - IP range aggregation
 - Percentiles Aggregation
 - Percentile Ranks Aggregation
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@markharwood
Copy link
Contributor

Many thanks for this.
To avoid our documentation getting outdated/broken we now run automated tests on example JSON provided in docs.
With this in mind can you add the relevant "CONSOLE" and "TESTRESPONSE" tags to your new examples and ensure they run cleanly? ( I can give you some pointers on how to run the tests if you need help). There are examples of these tags in the examples of the existing aggs documentation.

@sudo-suhas
Copy link
Contributor Author

@markharwood I saw those tags while browsing the docs but wasn't sure about their purpose. Additionally, I copied the documentation from Range Aggregation(which has docs for keyed) to ensure that I keep it similar. However, the Range Aggregation docs is missing these tags as well. I understand that I can run gradle check for these. Any other pointers, including gradle setup, would be very welcome.

@markharwood
Copy link
Contributor

It's an evolution we're on - runnable/testable docs is the goal and it will take us some time to get 100% coverage of old docs but we are aiming for all new docs to have this as part of the examples.
We have procedures in place to break the build if new doc changes fail to include workable examples.

gradle docs:check can be used to run the examples. If you write a deliberately bad example it will fail and the output will include a line that says "reproduce with" - this will include the name of the file and the line number etc that allows you to repeat the docs check but only for that one file that you want to test without having to run all the other examples. This should be much quicker.

The docs/build.gradle file includes sections to declare any index content you may need to setup as the subject content for the examples/tests but I'd try stick with the example content already referenced in the existing examples if possible. You might need to look at the example content to understand what will be in the response to your examples.

Thanks again

@sudo-suhas
Copy link
Contributor Author

@markharwood If I have the failure logs like so:

Tests with failures:cs:integTestRunner > Tests [842|2|30], in  642s completed DocsClientYamlTestSuiteIT
  - org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT.test {yaml=reference\search\request/highlighting/line_454}
  - org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT.test {yaml=reference\search\request/highlighting/line_388}

JUnit4 test failed, ant output was:
   [junit4] <JUnit4> says ??????! Master seed: 950C90FBC440FE
   [junit4] JVM J0:     0.67 ..   649.61 =   648.94s
   [junit4] Execution time total: 10 minutes 49 seconds
   [junit4] Tests summary: 1 suite, 842 tests, 2 failures, 30 ignored (30 assumptions)

:docs:integTestRunner FAILED
...
=========================================
:docs:integTestCluster#stop

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':docs:integTestRunner'.
> There were test failures: 1 suite, 842 tests, 2 failures, 30 ignored (30 assumptions) [seed: 950C90FBC440FE]

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 12 mins 14.498 secs

How do I run the gradle check only for the required file? I could not find the line with reproduce with instruction.

Running gradle :docs:integTestRunner takes a lot of time(~12 mins).

@markharwood
Copy link
Contributor

Here's an example from the last aggregation I worked on:
If I edit the file elasticsearch/docs/reference/aggregations/bucket/adjacency-matrix-aggregation.asciidoc and make the example JSON have a bad aggregation name e.g. adjacency_matriXXXXX

and then run gradle docs:check it fails.
The error output includes this line:

  2> REPRODUCE WITH: gradle :docs:integTestRunner -Dtests.seed=364E669D9C21AD2A -Dtests.class=org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT -Dtests.method="test {yaml=reference/aggregations/bucket/adjacency-matrix-aggregation/line_35}" -Dtests.security.manager=true -Dtests.locale=fr-BE -Dtests.timezone=Mexico/BajaSur

You can either repeat this for your example JSON or copy the gradle :docs:integTestRunner ... command above and edit the filenames/line numbers to work on your files/examples.

@sudo-suhas
Copy link
Contributor Author

Histogram Aggregation already has a section 'Response Format' which addresses the same topic. Should remove the added Keyed Response section.

@sudo-suhas
Copy link
Contributor Author

sudo-suhas commented Mar 28, 2017

You can either repeat this for your example JSON or copy the gradle :docs:integTestRunner ... command above and edit the filenames/line numbers to work on your files/examples.

This worked. It turns out, the reproduce with was printed but since there was so much log output, it got gobbled up.

@javanna javanna added >docs General docs changes review labels Apr 13, 2017
Add testable examples using indexes populated in build.gradle
  - Date Histogram Aggregation
  - Date Range Aggregation
  - Percentiles Aggregation
  - Percentile Ranks Aggregation
Remove redundant section `Keyed Response`
@sudo-suhas
Copy link
Contributor Author

sudo-suhas commented Apr 18, 2017

I have updated the docs with testable examples as requested with a couple of exceptions.
For Ip Range aggregation and Geo distance aggregation, no suitable example data is present.
Also, both are present in the buildRestTests.expectedUnconvertedCandidates in build.gradle.
Removed the Keyed Response section I had added as Response Format for the same is already present.

When I run gradle docs:check, I get the following failures:

Tests with failures:cs:integTestRunner > Tests [898|4|37], in  742s completed DocsClientYamlTestSuiteIT
  - org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT.test {yaml=reference\search/field-caps/line_19}
  - org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT.test {yaml=reference\search/field-caps/line_11}
  - org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT.test {yaml=reference\search/field-caps/line_80}
  - org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT.test {yaml=reference\search/field-caps/line_28}

I have not made any change to that file so merging this should not be a problem.

EDIT: Post merging master, I was able to see that Geodistance Aggregation had been updated and now had sample data. I used the same for updating the examples so that they are testable as well.

@javanna
Copy link
Member

javanna commented Apr 18, 2017

hi @sudo-suhas this was open a while ago, would you mind rebasing your PR against latest master please?

@javanna javanna self-assigned this Apr 18, 2017
@sudo-suhas
Copy link
Contributor Author

sudo-suhas commented Apr 18, 2017

There was a conflict in one of the files and I merged the master branch using the GUI while resolving it in github itself. Do I still need a rebase?

@javanna
Copy link
Member

javanna commented Apr 18, 2017

no sorry @sudo-suhas I didn't notice that. I will run tests then.

@javanna
Copy link
Member

javanna commented Apr 18, 2017

jenkins please test this

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

hi @sudo-suhas this looks great! I left a minor comment, if you can address that I can then merge this in.


Response:

Response:
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this "Response:" repetition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Pushed commit with the fix

@javanna javanna merged commit cee7629 into elastic:master Apr 18, 2017
javanna pushed a commit that referenced this pull request Apr 18, 2017
Add 'keyed' parameter documentation for following:
 - Date Histogram Aggregation
 - Date Range Aggregation
 - Geo Distance Aggregation
 - Histogram Aggregation
 - IP range aggregation
 - Percentiles Aggregation
 - Percentile Ranks Aggregation
@javanna javanna added the v5.5.0 label Apr 18, 2017
@sudo-suhas sudo-suhas deleted the docs_for_keyed_option branch April 18, 2017 14:00
javanna pushed a commit that referenced this pull request Apr 18, 2017
Add 'keyed' parameter documentation for following:
 - Date Histogram Aggregation
 - Date Range Aggregation
 - Geo Distance Aggregation
 - Histogram Aggregation
 - IP range aggregation
 - Percentiles Aggregation
 - Percentile Ranks Aggregation
@javanna javanna added the v5.4.0 label Apr 18, 2017
@javanna
Copy link
Member

javanna commented Apr 18, 2017

Thanks again @sudo-suhas I merged this to master and cherry-picked to 5.x and 5.4.

@sudo-suhas
Copy link
Contributor Author

Thank you for suggesting I make a pull request 😉

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.

4 participants