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

Framework to support W3C test-driven specification #2003

Closed

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Oct 8, 2021

Fixes #1210

There is a framework for specifications from the W3C, the W3C QA Framework, this is its abstract:

The goal of this document is to help W3C editors write better specifications, by making a specification easier to interpret without ambiguity and clearer as to what is required in order to conform. It focuses on how to define and specify conformance. It also addresses how a specification might allow variation among conforming implementations. The document presents guidelines or requirements, supplemented with good practices, examples and techniques.

What does this PR do?

  1. It changes one of the spec documents (specification/baggage/api.md) to make it follow the W3C QA framework. This change is intended to show how a document would look after being changed to follow the W3C QA Framework.
  2. It adds a script that can parse the spec documents that follow the W3C QA Framework and produce JSON files that can list the included requirements, how mandatory they are and their identifier.
  3. Adds a conformance clause for OpenTelemetry that details how the documents that form this specification are to be written.

Why did I create this PR?

While I was working on implementing Metrics, I found myself first having to read all the related documents to find the requirements so that I could know what to implement. This is something that should be as easy as looking for all instances of MUST, SHOULD, MAY, etc but it is not so because there is no guarantee that as the specification is written today every requirement is written in this way (please see this comment for examples). I found it necessary then to make this PR more strict in the way it is being written so that its requirements could be extracted from it.

What is the general strategy here?

With the approach suggested here we should be able to generate JSON files that contain all requirements so that they can be then transformed into test cases in each implementation. Once an implementation implements and passes all of the mandatory test cases (the ones whose corresponding RFC 2119 keyword is MUST [NOT] or equivalent) and it can consider itself compliant.

This idea will require of course rewriting all the documents, but this can be done little by little with one PR per document until we can have all the specification in this format. It is not necessary for this project to stop being developed to format all documents, it continue with some documents in this format and some not.

What about the compliance matrix?

The compliance matrix gathers all the features that each SIG has implemented and marks them as optional or not. I find it convenient but I don't think it is something that gives the level of detail that this approach gives. The compliance matrix is feature-oriented, this approach is requirement-oriented. I think both serve their specific purpose and can continue to exist together.

@ocelotl ocelotl force-pushed the test_driven_specification branch 5 times, most recently from ece225a to f87311f Compare October 8, 2021 18:44
@ocelotl ocelotl marked this pull request as ready for review October 8, 2021 18:50
@ocelotl ocelotl requested review from a team October 8, 2021 18:50
@tigrannajaryan
Copy link
Member

It would be useful to to start with the motivation for the change, what are the cons and pros and why we should adopt it.

This appears related to https://github.com/open-telemetry/opentelemetry-specification/pull/1211/files which did not gain traction. What has changed that we should consider this now?

@ocelotl ocelotl force-pushed the test_driven_specification branch 3 times, most recently from 3d61a40 to 62db85c Compare October 12, 2021 12:15
@ocelotl
Copy link
Contributor Author

ocelotl commented Oct 12, 2021

It would be useful to to start with the motivation for the change, what are the cons and pros and why we should adopt it.

Yes, good point:

Motivation for this change

The spec is hard to understand because it is written in an inconsistently-applied formal language. Here are some examples:

When implementing OpenTelemetry it is very hard to know what has to be implemented because there is no clear listing of all requirements. If the specifcation is written in the way intended in this PR it will be possible to generate automatically a list of every requirement and condition that the specification states.

It is also much easier to check that the specification is correct if we only need to check a list of well defined sections. Some checks (like having a RFC 2119 keyword, for example) can even be automated.

This appears related to https://github.com/open-telemetry/opentelemetry-specification/pull/1211/files which did not gain traction. What has changed that we should consider this now?

Specification-wise, nothing. The specification continues to have the same problems it had when #1211 was opened. This PR now introduces support to parse conditions and conditional requirements.

To illustrate better what this PR attempts to achieve, here is the generated JSON file for the baggage/api.md document as changed in this PR:

[
    {
        "id": "Requirement 1",
        "content": "Each name in `Baggage` MUST be associated with exactly one value.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 2",
        "content": "The order of name/value pairs in the `Baggage` MUST NOT be significant.",
        "RFC 2119 keyword": "MUST NOT"
    },
    {
        "id": "Requirement 3",
        "content": "The API MAY implement these functions by interacting with the baggage via the `Context` directly.",
        "RFC 2119 keyword": "MAY"
    },
    {
        "id": "Requirement 4",
        "content": "The API MUST be fully functional in the absence of an installed SDK.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 5",
        "content": "The `Baggage` container MUST be immutable.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 6",
        "content": "The API MUST provide a `getValue` function that takes a required name parameter and returns a value associated with the given name, or null if the given name is not present.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 7",
        "content": "The API MUST provide a `getAllValues` function that returns either an immutable collection of name/value pairs on the `Baggage` or an iterator on such collection.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 8",
        "content": "The API MUST provide a `setValue` function which takes a required name, a required value and optional metadata and returns a new `Baggage` that contains the new value.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 9",
        "content": "The API MAY implement the `setValue` function by using a `Builder` pattern and exposing a way to construct a `Builder` from a `Baggage`.",
        "RFC 2119 keyword": "MAY"
    },
    {
        "id": "Requirement 10",
        "content": "The optional `setValue` metadata parameter SHOULD be an opaque wrapper for a string with no semantic meaning.",
        "RFC 2119 keyword": "SHOULD"
    },
    {
        "id": "Requirement 11",
        "content": "To delete a name/value pair, the API MUST provide a `removeValue` function which takes a required name and returns a new `Baggage` which no longer contains the selected name.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 12",
        "content": "The API MAY implement the `removeValue` function by using a `Builder` pattern and exposing a way to construct a `Builder` from a `Baggage`.",
        "RFC 2119 keyword": "MAY"
    },
    {
        "id": "Requirement 13",
        "content": "The API SHOULD NOT provide access to the its\n[Context Key](../context/context.md#create-a-key).",
        "RFC 2119 keyword": null
    },
    {
        "id": "Requirement 14",
        "content": "The API MUST provide a way to remove all baggage entries from a context.",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 15",
        "content": "The API or an extension package MUST include a `TextMapPropagator` which implements the [W3C Baggage Specification](https://w3c.github.io/baggage)[^1].",
        "RFC 2119 keyword": "MUST"
    },
    {
        "id": "Requirement 16",
        "content": "On `extract`, a propagator SHOULD store all metadata as a single metadata instance per entry.",
        "RFC 2119 keyword": "SHOULD"
    },
    {
        "id": "Requirement 17",
        "content": "On `inject`, a propagator SHOULD append the metadata per the W3C specification format.",
        "RFC 2119 keyword": "SHOULD"
    },
    {
        "id": "Condition 1",
        "content": "The API does not operate directly on the `Context`.",
        "children": [
            {
                "id": "Conditional Requirement 1.1",
                "content": "The API MUST provide an `extract` function to extract the `Baggage` from a`Context` instance.",
                "RFC 2119 keyword": "MUST"
            },
            {
                "id": "Conditional Requirement 1.2",
                "content": "The API MUST provide an `inject` function to inject the `Baggage` in a`Context` instance.",
                "RFC 2119 keyword": "MUST"
            }
        ]
    },
    {
        "id": "Condition 2",
        "content": "The language has support for implicitly propagated `Context`(see [here](../context/context.md#optional-global-operations)).",
        "children": [
            {
                "id": "Conditional Requirement 2.1",
                "content": "The API SHOULD provide a get `Baggage` functionality to get the currently active`Baggage` from the implicit context and a set `Baggage` functionality to set thecurrently active `Baggage` into the implicit context.",
                "RFC 2119 keyword": "SHOULD"
            },
            {
                "id": "Condition 2.1",
                "content": "The API provides a functionality to get the currently active `Baggage` from theimplicit context and a functionality to set the currently active `Baggage` intothe implicit context.",
                "children": [
                    {
                        "id": "Conditional Requirement 2.1.1",
                        "content": "The get `Baggage` functionality behavior MUST be equivalent to getting theimplicit context, then extracting the `Baggage` from the context.",
                        "RFC 2119 keyword": "MUST"
                    },
                    {
                        "id": "Conditional Requirement 2.1.2",
                        "content": "The set `Baggage` functionality behavior MUST be equivalent to getting theimplicit context, then inserting the `Baggage` into the context.",
                        "RFC 2119 keyword": "MUST"
                    },
                    {
                        "id": "Conditional Requirement 2.1.3",
                        "content": "The get and set `Baggage` functionalities MUST operate solely on the contextAPI.",
                        "RFC 2119 keyword": "MUST"
                    },
                    {
                        "id": "Conditional Requirement 2.1.4",
                        "content": "The get and set `Baggage` functionalities MAY be exposed- as static methods on the baggage module or- as static methods on a class inside the baggage module or- on the `Baggage` class",
                        "RFC 2119 keyword": "MAY"
                    },
                    {
                        "id": "Conditional Requirement 2.1.5",
                        "content": "The get and set `Baggage` functionalities SHOULD be fully implemented inthe API.",
                        "RFC 2119 keyword": "SHOULD"
                    }
                ]
            }
        ]
    }
]

@ocelotl ocelotl changed the title Test driven specification Framework to support W3C test-driven specification Oct 13, 2021
@jmacd
Copy link
Contributor

jmacd commented Oct 13, 2021

I am a really big fan of this work, @ocelotl!

The OpenTelemetry specification is expansive and written by many authors. As we see from your work on #1956, it is difficult if not impossible to construct a compliance matrix from the text of the specification without having the authors and the community members there to help with interpretation.

It looks like this can be done iteratively: one step at a time, a part of the specification will be rewritten to match the style given here, at which point the compliance matrix row headings can be automatically checked or generated.

Because this is a large PR, I want to encourage you to first (sorry, procedural nits!) file an issue describing the problem "Make the OpenTelemetry specification automatically testable" or something similar. You've already written the motivation above, but we use issues to prioritize discussions about large changes ahead of actual pull requests, usually.

Please file an issue that repeats the motivation and also gives a brief explanation of the steps, e.g., (1) merge the spec-writing guidelines, (2) update document status states to manage the transition, (3) merge the tools for generating x/y/z (4) finally for each component of the specification, (a) refactor, (b) regenerate compliance matrix, (c) ...

@ocelotl
Copy link
Contributor Author

ocelotl commented Oct 14, 2021

update document status states to manage the transition

Thanks for the feedback, @jmacd ✌️

I have updated #1210 to address your comments.

@ocelotl ocelotl force-pushed the test_driven_specification branch from 62db85c to b503896 Compare October 18, 2021 12:09
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 11, 2021
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 19, 2021
@hdost
Copy link

hdost commented Dec 28, 2021

Is this something that's still being worked on, but maybe elsewhere?

@ocelotl
Copy link
Contributor Author

ocelotl commented Jan 4, 2022

Is this something that's still being worked on, but maybe elsewhere?

Not that I am aware of, curious why you ask 😅

jmacd pushed a commit to jmacd/opentelemetry-specification that referenced this pull request Jan 18, 2022
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.

Make it possible to automatically list all specification requirements
5 participants