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

Introduce stability description to the REST API specification #38413

Merged
merged 17 commits into from
Jun 12, 2019

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Feb 5, 2019

This introduces a state key to the REST API specs to signal an API's readiness:

  • state indicating the state of the API, defaults to stable
    • private this API should not be be implemented by clients
    • experimental highly likely to break in the near future (minor/path), no bwc guarantees.
      Possibly removed in the future.
    • beta less likely to break or be removed but still reserve the right to do so
    • stable No backwards breaking changes in a minor (default if not specified)

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Feb 5, 2019

This could in the future also include e.g deprecated if we want to remove _source or other API's. Although no such plans currently exists.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Feb 5, 2019

For deprecations I prefer @mayya-sharipova proposal here: #35262

@jtibshirani jtibshirani added the :Core/Infra/REST API REST infrastructure and utilities label Feb 6, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@Mpdreamz
Copy link
Member Author

Discussed during es-core-infra sync.

  • private should not be a state. If an API is considered private it should not be in the json rest spec.
    • There are a couple of SAML endpoints undocumented as well that might warrant additional discussion (are they private?). How do we stay vigilant an API is not private by accident?
  • state property name is not a great name, either status or stability.
  • Ideally there is a test that enforces the new property only uses the enum values we settled on experimental, beta and stable, or empty defaulting to stable.

@Mpdreamz
Copy link
Member Author

  • state property name is not a great name, either status or stability.

Addressed this item.

@jkakavas
Copy link
Member

There are a couple of SAML endpoints undocumented as well that might warrant additional discussion (are they private?). How do we stay vigilant an API is not private by accident?

We want to document these two for some time. I'll open an issue for this so that folks can track process as we've been answering this question every once in a while, but I plan on tackling the docs in the next couple of weeks

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Mar 4, 2019

Awesome to hear @jkakavas looking forward to those 👍

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Mar 4, 2019

run elasticsearch-ci/2

@jaymode jaymode self-requested a review March 27, 2019 15:38
Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a good change. Ultimately I would like to see stability be a field that is required to be defined so that APIs do not get accidentally marked as stable due to leaving this field out. Additionally, there is at least one stable API that is not stable in terms of the response body, see #40104. Should we have a separate type for these?

@@ -1,6 +1,7 @@
{
"ccr.delete_auto_follow_pattern": {
"documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/current/ccr-delete-auto-follow-pattern.html",
"stability" : "beta",
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 all CCR APIs are GA #39722

Copy link
Member Author

Choose a reason for hiding this comment

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

ta, this PR preceeded that one. Updated now.

@@ -1,6 +1,7 @@
{
"sql.clear_cursor": {
"documentation": "Clear SQL cursor",
"stability" : "beta",
Copy link
Member

Choose a reason for hiding this comment

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

SQL APIs should be GA as of 6.7. @elastic/es-sql can you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, SQL is GA since 6.7. Thanks @jaymode for picking it up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, to reflect latest state changes. Thanks for confirming.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Apr 4, 2019

cc @elastic/es-clients thoughts on the effect of #40104 on this PR?

What should we call this type of API? its stable in terms of url/query_params but the body is reserved to be broken in between versions.

This API (GET /_cluster/state) is now for inspection purposes only and not an API users should be expected to write features against.

cc @elastic/microsoft #40104 has deep implications on how we map ClusterStateResponse in 7.9

@russcam
Copy link
Contributor

russcam commented Apr 4, 2019

It looks like this would be a candidate for the private state, or the REST spec for it ought to be removed

@karmi
Copy link
Contributor

karmi commented Apr 7, 2019

Here's a good example, in #39382 (emphasis mine):

This PR adds an internal REST API for querying context information about Painless whitelists.

The API specification doesn't include the documentation link even, so this one looks a very good candidate for private / internal API.

(Note that the client generators still need a documentation link.)

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Apr 8, 2019

I opted for internal to represent the guarantees the cluster state API makes.

Its here to stay but we no longer make guarantees about its output staying the same during the course of a major version.

This is different then private IMO since these can be removed at any time during the lifetime of a major version.

@jaymode
Copy link
Member

jaymode commented Apr 8, 2019

@DaveCTurner @jasontedor would love to get your input on the stability listed for the cluster state API and whether that matches your intentions.

@DaveCTurner
Copy link
Contributor

Yes, internal sounds right to me amongst those choices. However I'd like to question whether we need to distinguish internal, beta and experimental at all. Although I see they have slightly different intentions, in practice I think they all really mean the same thing: "this API may change in incompatible ways in future, consume it at your own risk".

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Apr 8, 2019

I would not want to oversimplify states too much:

beta expirimental signal features are in development towards stable but might be removed if we please.
private signals a client should not implement these at all, see e.g the data_frame json stubs introduced in #40283 on master
internal means this API is here to stay but the request/response do not follow normal bwc guarantees.

@jaymode I've amended the PR so that not specifying stability fails the yaml tests.

Also if someone specifies a stability that is not known the yaml tests will fail too stating the API which has the offending spec.

@karmi
Copy link
Contributor

karmi commented Apr 8, 2019

I agree with @Mpdreamz here — the distinction between experimental|beta, private and internal is important.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Apr 9, 2019

run elasticsearch-ci/2

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

…ility appears after documentation everywhere
@Mpdreamz
Copy link
Member Author

run elasticsearch-ci/1

@rjernst
Copy link
Member

rjernst commented Apr 24, 2019

The distinction is there because sometimes an API should be exposed in the client such as cluster.state.json with no backwards guarantees on the response which makes it internal.

I think this can be achieved in a different way. The api is "stable" in that the cluster state handler is not going away. It is the content that is unstable, ie the data returned by the cluster state api is an opaque blob.

The private was reintroduced seeing that sometimes stubs get added for documentation/development purposes e.g #40283.

I don't think those should have rest api specs at all. They can exist as http handlers in the code, and used eg for documentation generation, but not exist in the spec. That was the same conclusion we agreed on in the original discussion regarding private apis.

@Mpdreamz
Copy link
Member Author

The semantics between internal and stable mean a lot to us clients.

It basically signals the difference between map these requests and responses or not.

internal to me means the response is an untyped blob/hash/map/dictionary of json but the API url and query parameters are stable and won't introduce breaking changes.

The progression of an API would be:

experimental => beta => internal or experimental => beta => stable

The meaning of internal is important here because experimental and beta API's will need to be delivered through add-on packages with their own versioning because many clients can not introduce breaking changes in minors.

private is now on:

  • data_frame.* API's
  • scripts_painless_context
  • monitoring.bulk

Given that we feel data_frame should have never been committed if they are stubs, and we are happy to role scripts_painless_context and monitoring.bulk under experimental I'd be happy to remove private.

@Mpdreamz
Copy link
Member Author

An alternative option would be to mark

cluster.state as stable and mark the respose in the spec as response_blob: true

The dataframe.* specs were added to make them available in kibana during testing development which brings back private as a possible state or we ca rename private to internal(given we mark cluster.state as stable) which would make the progression:

internal => expirimental => beta => stable

With internal not being exposed at all, expirimental and beta being delivered out of band for clients that can not break in between major versions and stable going into the main packages.

@rjernst
Copy link
Member

rjernst commented May 6, 2019

cluster.state as stable and mark the respose in the spec as response_blob: true

This is what I was suggesting, and I think it is simpler and more clear.

The dataframe.* specs were added to make them available in kibana during testing development which brings back private as a possible state

It is unclear how kibana can do testing against an api that is considered "private", as anything returned from it could possibly change at any time and break tests. I feel strongly these private or internal apis should simply not exist within our spec files, as the spec files by definition are the apis we expect users to use.

@hendrikmuhs
Copy link
Contributor

@Mpdreamz

The data_frame.* API's should be beta

I know this hasn't been the case when you opened this PR, but meanwhile it progressed to beta (master and 7x)

@codebrain codebrain changed the title introduce state to the REST API specification Introduce stability description to the REST API specification May 24, 2019
@Mpdreamz
Copy link
Member Author

Addressed remaining PR comments:

  • data frame API's are now stable ty @hendrikmuhs 👍
  • cluster.state is now stable and the response is marked to be treated as key value like so
"response": {
  "treat_json_as_key_value" : true
}
  • remove internal and private as stability states.

There are 2 API's that are not really intended for public consumption though:

I wholeheartedly subscribe to not implementing private/internal API's and in effect blessing the java client as the defacto client e.g both of those API's are useful from other ecosystems too (beats/logstash/codesearch/apm et all). That said I changed these to be documented as experimental for now but that might not be the most accurate description of its intent either. Would love some more thoughts on how to address this @rjernst @elastic/es-clients and anyone else 👍

@rjernst
Copy link
Member

rjernst commented Jun 11, 2019

Thanks @Mpdreamz! I opened #43122 to move the painless spec file into the appropriate test, so it will no longer be found here by clients using the spec.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM other than the merge conflict due to the painless context api being moved.

@Mpdreamz Mpdreamz merged commit 61c34bb into elastic:master Jun 12, 2019
Mpdreamz added a commit to Mpdreamz/elasticsearch that referenced this pull request Jun 17, 2019
…c#38413)

* introduce state to the REST API specification

* change state over to stability

* CCR is no GA updated to stable

* SQL is now GA so marked as stable

* Introduce `internal` as state for API's, marks stable in terms of lifetime but unstable in terms of guarantees on its output format since it exposes internal representations

* make setting a wrong stability value, or not setting it at all an error that causes the YAML test suite to fail

* update spec files to be explicit about their stability state

* Document the fact that stability needs to be defined

Otherwise the YAML test runner will fail (with a nice exception message)

* address check style violations

* update rest spec unit tests to include stability

* found one more test spec file not declaring stability, made sure stability appears after documentation everywhere

* cluster.state is stable, mark response in some way to denote its a key value format that can be changed during minors

* mark data frame API's as beta

* remove internal and private as states for an API

* removed the wrong enum values in the Stability Enum in the previous commit

(cherry picked from commit 61c34bb)
Mpdreamz added a commit to Mpdreamz/elasticsearch that referenced this pull request Jun 17, 2019
…c#38413)

* introduce state to the REST API specification

* change state over to stability

* CCR is no GA updated to stable

* SQL is now GA so marked as stable

* Introduce `internal` as state for API's, marks stable in terms of lifetime but unstable in terms of guarantees on its output format since it exposes internal representations

* make setting a wrong stability value, or not setting it at all an error that causes the YAML test suite to fail

* update spec files to be explicit about their stability state

* Document the fact that stability needs to be defined

Otherwise the YAML test runner will fail (with a nice exception message)

* address check style violations

* update rest spec unit tests to include stability

* found one more test spec file not declaring stability, made sure stability appears after documentation everywhere

* cluster.state is stable, mark response in some way to denote its a key value format that can be changed during minors

* mark data frame API's as beta

* remove internal and private as states for an API

* removed the wrong enum values in the Stability Enum in the previous commit

(cherry picked from commit 61c34bb)
Mpdreamz added a commit that referenced this pull request Jun 17, 2019
#43278)

* introduce state to the REST API specification

* change state over to stability

* CCR is no GA updated to stable

* SQL is now GA so marked as stable

* Introduce `internal` as state for API's, marks stable in terms of lifetime but unstable in terms of guarantees on its output format since it exposes internal representations

* make setting a wrong stability value, or not setting it at all an error that causes the YAML test suite to fail

* update spec files to be explicit about their stability state

* Document the fact that stability needs to be defined

Otherwise the YAML test runner will fail (with a nice exception message)

* address check style violations

* update rest spec unit tests to include stability

* found one more test spec file not declaring stability, made sure stability appears after documentation everywhere

* cluster.state is stable, mark response in some way to denote its a key value format that can be changed during minors

* mark data frame API's as beta

* remove internal and private as states for an API

* removed the wrong enum values in the Stability Enum in the previous commit

(cherry picked from commit 61c34bb)
Mpdreamz added a commit that referenced this pull request Jun 17, 2019
#43279)

* introduce state to the REST API specification

* change state over to stability

* CCR is no GA updated to stable

* SQL is now GA so marked as stable

* Introduce `internal` as state for API's, marks stable in terms of lifetime but unstable in terms of guarantees on its output format since it exposes internal representations

* make setting a wrong stability value, or not setting it at all an error that causes the YAML test suite to fail

* update spec files to be explicit about their stability state

* Document the fact that stability needs to be defined

Otherwise the YAML test runner will fail (with a nice exception message)

* address check style violations

* update rest spec unit tests to include stability

* found one more test spec file not declaring stability, made sure stability appears after documentation everywhere

* cluster.state is stable, mark response in some way to denote its a key value format that can be changed during minors

* mark data frame API's as beta

* remove internal and private as states for an API

* removed the wrong enum values in the Stability Enum in the previous commit

(cherry picked from commit 61c34bb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.