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

Move README status table to separate machine-readable file #19175

Closed
4 of 6 tasks
mx-psi opened this issue Mar 2, 2023 · 16 comments
Closed
4 of 6 tasks

Move README status table to separate machine-readable file #19175

mx-psi opened this issue Mar 2, 2023 · 16 comments
Labels
ci-cd CI, CD, testing, build issues cmd/mdatagen mdatagen command documentation Improvements or additions to documentation enhancement New feature or request priority:p2 Medium Stale

Comments

@mx-psi
Copy link
Member

mx-psi commented Mar 2, 2023

Component(s)

No response

Describe the issue you're reporting

All components must have a status table describing their stability, supported pipeline types, available distributions and warnings (see #19172 for the latter). This is useful for end-users to understand how to use a component.

Currently, this table is on each component's README file which makes it difficult to ingest this information. I propose moving this table to a separate machine readable file so that

  • we can validate its contents (e.g. validate presence of required fields, validate their values, in the future try to match the metadata with the actual code)
  • it's easier to build automated tooling around it, for example to
    • include status information on the component registry,
    • help the community understand what components are available on contrib and what their capabilities are
    • automatically validate OCB manifests (e.g. forbid components with a given warning or below a given stability level)
    • in the future, help validate that a component can be added to the registry even if not living on contrib

We can still generate the README table from the machine-readable file by placing a comment marking the position of the table.

To kickstart this we would need to:

Future work may include

  • Validation of code against schema (e.g. supported pipeline types or stability)
  • Adding information other than status to this file (e.g. description or statefulness)
@mx-psi mx-psi added documentation Improvements or additions to documentation enhancement New feature or request priority:p2 Medium ci-cd CI, CD, testing, build issues labels Mar 2, 2023
@djaglowski
Copy link
Member

djaglowski commented Mar 2, 2023

Taking this a step further, wouldn't it make sense to have such a machine readable file for all standardized parts of the readme?

For example:

class: processor
type: transform
description: modifies telemetry based on configuration using the [OpenTelemetry Transformation Language](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/ottl)
stability:
  traces: beta
  metrics: alpha
  logs: alpha
distributions: [contrib]
stateful: true # See https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/18878
warnings: # See https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/19172
-  unsound_transformations
-  identity_conflict
-  orphaned_telemetry

@TylerHelmuth
Copy link
Member

Something like this would also be very useful for open-telemetry/opentelemetry-collector#7274

@TylerHelmuth
Copy link
Member

And I'll argue that Stateful fits into Warnings and doesn't need its own section to indicate Stateless. But I'm not willing to die on that hill.

@sh0rez
Copy link
Member

sh0rez commented Mar 2, 2023

happy to look into this if no one else already does!

@codeboten
Copy link
Contributor

I started looking at adding this to metadata.yaml a little while ago. I'd suggest we start incrementally with just the status. It would be great to use this information in the code as well, rather than having to set this in multiple places.

@atoulme
Copy link
Contributor

atoulme commented Mar 2, 2023

I support this.

@mx-psi
Copy link
Member Author

mx-psi commented Mar 2, 2023

I added a couple items to 'future work' on the original post including @djaglowski and @codeboten's suggestions. I agree with @codeboten that we can start with just the status and work from there.

I started looking at adding this to metadata.yaml a little while ago.

@codeboten one open question for me if we re-use metadata.yaml is what to do with components like the hostmetrics receiver that do not have a single metadata.yaml file but rather one per scraper. It sounds to me like we should modify the metadata.yaml schema for this to work. Do you have any thoughts on that?

happy to look into this if no one else already does!

@sh0rez Sure :) I think we can start with only the status table and open a PR to do this on a few components to see how things work out in practice. I suggest you pick some that don't already have the existing metadata.yaml file so that we can continue discussing how to unify them both here. I am happy to help if you need anything.

@codeboten
Copy link
Contributor

@mx-psi @sh0rez here's sorta what i had in mind to get us started: #19178

I was planning on adding a generated_status.go file as part of that change. If folks are ok with this as a starting point, i'll finish the PR later today and put it up for review

codeboten pushed a commit to codeboten/opentelemetry-collector-contrib that referenced this issue Mar 2, 2023
This is a proposed first step to remove needing to generate a table for each component separately. As part of this change I propose we also use the metadata to auto-generate the code to return the component status.

Link issue: open-telemetry#19175

Signed-off-by: Alex Boten <aboten@lightstep.com>
@dmitryax
Copy link
Member

dmitryax commented Mar 6, 2023

@codeboten one open question for me if we re-use metadata.yaml is what to do with components like the hostmetrics receiver that do not have a single metadata.yaml file but rather one per scraper. It sounds to me like we should modify the metadata.yaml schema for this to work. Do you have any thoughts on that?

I don't like how we split the scrapers in the hostmetrics receiver. I think we should have every component have only one metadata.yaml file. And I would like to suggest replacing the scrapers concept of hostmetrics receiver with metric groups that can be disabled/enabled explicitly the same way as currently done for metrics. (btw this will solve the issue in the helm chart where we cannot disable a scraper enabled by default cc @TylerHelmuth). @codeboten @djaglowski @mx-psi Let me know if you agree, and I'll create an issue to discuss the config interface.

@mx-psi
Copy link
Member Author

mx-psi commented Mar 7, 2023

Makes sense to me @dmitryax, I think with that cleared out it's clear to me that this should go in mdatagen and just be the same file.

@dmitryax
Copy link
Member

I submitted #19440. Let me know if the description is unclear, and if I should elaborate more on some parts. Any suggestions/objections are welcome.

codeboten pushed a commit to codeboten/opentelemetry-collector-contrib that referenced this issue Mar 17, 2023
This is a proposed first step to remove needing to generate a table for each component separately. As part of this change I propose we also use the metadata to auto-generate the code to return the component status.

Link issue: open-telemetry#19175

Signed-off-by: Alex Boten <aboten@lightstep.com>
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label May 9, 2023
@mx-psi mx-psi removed the Stale label May 9, 2023
@mx-psi
Copy link
Member Author

mx-psi commented May 9, 2023

Let's keep this is open as a meta issue about metadata.yaml, will try to keep the description updated with all related issues.

@mx-psi mx-psi added the cmd/mdatagen mdatagen command label May 11, 2023
@github-actions
Copy link
Contributor

Pinging code owners for cmd/mdatagen: @dmitryax. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Aug 14, 2023
@djaglowski
Copy link
Member

It seems the main effort is complete. Should we close this one?

@mx-psi mx-psi closed this as completed Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cd CI, CD, testing, build issues cmd/mdatagen mdatagen command documentation Improvements or additions to documentation enhancement New feature or request priority:p2 Medium Stale
Projects
None yet
Development

No branches or pull requests

7 participants