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

CI for docs' code snippets #3633

Closed
ekquasar opened this issue Dec 1, 2023 · 10 comments
Closed

CI for docs' code snippets #3633

ekquasar opened this issue Dec 1, 2023 · 10 comments
Labels
CI/infra CI & infrastructure discussion Input from everyone is helpful to drive this forward

Comments

@ekquasar
Copy link
Contributor

ekquasar commented Dec 1, 2023

What's wrong?

The docs are currently include broken code. This is harmful for the adoption of OpenTelemetry as it degrades the project's reputation and ability of users to onboard. See this earlier discussion for more detail.

How to fix?

Let's get CI for the code snippets in the docs so we can ensure they work - or at the very least improve visibility for when they're broken.

Where to start?

I'll propose starting with the Python "Getting Started" docs as that's where I can probably help, and the issue with those docs is documented.

@svrnm svrnm added CI/infra CI & infrastructure discussion Input from everyone is helpful to drive this forward labels Dec 1, 2023
@svrnm
Copy link
Member

svrnm commented Dec 1, 2023

Thank you for raising this, I will share some thoughts on this later as well.

@open-telemetry/docs-approvers please take a look as well and share your thoughts

@ekquasar sending this upfront: this is not a trivial task, and a major reason for us not taking this on yet, is that this requires a lot of dedicated time and resources to do this right. We discussed it a few times during SIG meetings, but always dropped it because of other priorities. So, I am happy to discuss this, but eventually someone needs to do the real work for it.

@ekquasar
Copy link
Contributor Author

ekquasar commented Dec 1, 2023

I wonder if there's not an incremental path to right, as such. If we get the desirables defined and the problem decomposed, there should be an MVP that stands out as an achievable result within a Sprint's worth of effort, I imagine.

I'm looking forward to understanding the issues here, because broken documentation is a critical issue.

@svrnm
Copy link
Member

svrnm commented Dec 1, 2023

This can and should(!) be done incrementally for sure. Let me try to express some of the requirements & challenges we face here.

Keep code & docs in sync

The first decision we need to make is how we keep code & docs in sync, there are a few options, all have their downsides and upsides:

  1. Separate code & docs, where code only is the final state of an example (e.g. in the case of the python code, the step where you have the instrumented app), checks are run against that and it is manually made sure that they are in sync (some SIGs (go?) do this for their examples already). While this is super easy to accomplish it create a lot of additional maintenance overhead.
  2. Separate code & docs, where the code gets synced into the docs somehow. There are multiple ways how that somehow can be accomplished, docsy has tooling for that, see here. This is slightly better than (1), but it makes it harder for new comers to contribute, as they need to learn yet another macro.
  3. Keep the code in the docs and find tooling that can extract the code to a pre-defined location. I remember that I saw some tools that can do that. This removes the hurdle for newcomers to learn about macros, and at the same time it keeps code&docs close together.

I personally prefer (3), can see (2) as a viable alternative as well, but I am against (1) or any other solution that increases overhead.

Please keep the numbers in mind as I will use them throughout the rest of the issue.

Where does the code live?

This could be a different repo (see open-telemetry/community#1474) or a dedicated folder in this repository. For the checks it is probably better to keep it "here".

Check all the code

If we implement something like (2) or (3) we end up with directory of code snippets, but they are not necessarily runnable: in some docs we have snippets not intended for running, in some docs we have "add the following 2 lines to your code", in some places we have the same file but different "add the following changes" applied, e.g. with the exporters. So, there needs to be logic stored somewhere that encodes the verbal instructions from the docs, to something runnable. This is easer with (2), since you can put the code together first and then reference those files. For (3) you can annotate the code fences, like code highlighting with hugo: https://gohugo.io/content-management/syntax-highlighting/:

```python { write your instructions here }
code
```

This may be trivial with the getting started, but will get much harder with more complex pages. Here of course the "incremental" comes to play: we don't have to solve all of that immediately, but we need to make sure all of that is anticipated.

Incremental implementation

As you said, this can (and should) be implemented incrementally. If we go with (2) this is given implicitly, if we go with (3) we need to annotate files accordingly.

Although, what I want to avoid is that we implement this for one page (Python Getting Started) and then stop with that because of lost interest, lack of bandwidth for it, and we have a one off solution. I would at least expect all getting started docs being covered.

Run the code in CI

To run the checks via github CI we need to pack things into docker containers/make sure the code runs in a container, so it has to be clear what base image is used, etc. for local verification.

Ideally we run the code checks to verify PRs before they get committed. In that case it is essential that this happens in a secure manner to avoid that someone injects something into the pipeline that way

The CI itself adds a layer of complexity to contributors. The current checks we apply on each PR are already overwhelming for many contributors, so for me it is important that we make sure that these changes are also with minimal impact, e.g. we only run them if code was changed (which creates a preference for (2)).

The nasty part

Ending with the biggest challenge: the output of the code needs to be checked, so while it might be a good start to check for "is the syntax OK", "does it compile", "does it run without error", we eventually also want to verify if the output is as expected (are spans, logs, metrics exported as expected). Those outputs are of course different every time the test is run, and (this is the nasty part) the language-specific SDKs/the collector get updated regularly, we pull in version updates and need to keep versions up-to-date for these checks. This will sometimes create a lot of breaks, issues, headaches. What I want to avoid is that we either hold back to update versions on the website because of breaking checks, or that we dismiss the checks eventually because we can not keep up.

I don't say that this is impossible, I just want to make sure we keep that in mind

Final thoughts

I am all in for having the code checked. We need this eventually! But at the same time it's my responsibility as maintainer to make sure that we are not overwhelming ourselves with additional work.

@ekquasar
Copy link
Contributor Author

ekquasar commented Dec 1, 2023

The separation of concerns in (1) is enough to give it my vote.

CI on separate example code doesn't need to be enforced - at the very least it will provide a minimum of visibility into the status of the documentation. Currently, the status of code examples is completely unknown a priori.

Where there are "illustrative" code snippets that are not intended to be run "as-is", they should be clearly marked as such. Where there is a narrative document with disjoint code examples, a link to the "full, working example code" at the top of the document in an info-box would be sufficient.

I'm not familiar with the CI for this project, but dockerizing the example code is trivial. FROM ubuntu:latest reproduces all the issues I've found on my workstation. As far as injection is concerned, as long as the test stages are separate from the build stages, could we rely on the CI provider's separation to keep malicious test code away from any distributed software? Is there a "Security Buddy" system in this community? An early review of 1..3 could help here.

Checking the output can be done incrementally, starting from the most flexible output checks e.g. first just check for valid JSON. This seems trivial but would catch existing errors like this.

An attempt at scoping

We don't need to enforce testing on every snippet across the site. Instead, we should work towards at least knowing when at least one code example works.

IMHO the most valuable minimal example is:

  • Flask app instrumented with traces and metrics
  • App exports to console
  • Test for existence of some valid JSON in console

If it's advertised up top that there is a quick-start example that works out of the box, this could alleviate a painful developer time-to-value issue.

@chalin
Copy link
Contributor

chalin commented Dec 4, 2023

As @svrnm mentioned, this is a known issue, that has been discussed in the past, see:

This repo briefly used a simple include shortcode, but that falls short of what we really need IMHO.

I have had a solution on the back burner (which is successfully being used in other projects), and would be glad to work on a similar feature in the context of OTel if @open-telemetry/docs-maintainers feel that it's a priority.

In a nutshell, the approach relies on code repos to have CI to ensure that their example code is functional. In this repo we'd use tooling to extract example-code snippets from the CI-tested code of the code repos. The extracted code snippets are caches, so we don't always need to extract from the code repo.

What would help me is if you could identify a few target code-snippets in the docs, and point me to the corresponding code in a code repo. I could work on prototyping the code-snippet sync for a few examples to start off with.

(I'll look into the tooling as soon as I can, but most likely I won't have much time until the new year.)

@cartermp
Copy link
Contributor

cartermp commented Dec 4, 2023

What I will say in all of this is that we need end-user contributions to still be as easy as possible. That's what moved us away from CI-checked samples at Microsoft: it was just too hard for anyone who didn't work in the docs org to update and improve things over time. I really wouldn't support something as confusing as multi-repo setups.

@ekquasar
Copy link
Contributor Author

ekquasar commented Dec 4, 2023

These sophisticated solutions would do wonders for improving the reliability of the documentation. But I wonder if something much simpler might 80/20 the time-to-value issue: keeping a single, end-to-end, working example CI-tested.

@chalin
Copy link
Contributor

chalin commented Dec 4, 2023

keeping a single, end-to-end, working example CI-tested.

I believe that the demo (or a subset thereof) is meant to serve that purpose. Smaller-scale examples would be good too.

Once you have an end-to-end example, inevitably one ends up wanting to refer to code snippets from the docs, so we're back to the need of keeping the docs and code snippets in sync.

We discussed this during the Comms meeting. I proposed to drop a PR (early next year) that illustrates how code snippets can be kept in sync; I'll probably start with go/example/dice since the snippets we'd need are in code repo already. (An alternative would be do use the demo docs.)

@chalin
Copy link
Contributor

chalin commented Dec 4, 2023

That's what moved us away from CI-checked samples at Microsoft

I agree that we don't want to add CI code checks to this repo. In that sense, could we close this issue in favor of #1635 (@svrnm was looking for #1635 but didn't find it at the time) and continue the code-snippet-sync discussion over there? @svrnm WDYT?

@svrnm
Copy link
Member

svrnm commented Dec 4, 2023

Yes, let's continue this discussion on #1635, copy/link the items you want to continue discussing there

@svrnm svrnm closed this as completed Dec 4, 2023
@open-telemetry open-telemetry locked as resolved and limited conversation to collaborators Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI/infra CI & infrastructure discussion Input from everyone is helpful to drive this forward
Projects
None yet
Development

No branches or pull requests

4 participants