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

[processor/transform] Added warnings sections #11052

Merged
merged 10 commits into from
Jun 27, 2022

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Jun 15, 2022

Description:
As discussed in the SIG today, we need a way to inform users of potential risks with any given component. This PR proposes a new "Warnings" section in the status head that can be used to link to a detailed list of warnings.

Transform Processor

Status
Stability [alpha]
Supported pipeline types traces, metrics, logs
Distributions [contrib]
Warnings Identity Crisis, Unsound Transformations, Orphaned Telemetry, Other

I think we could standardize the values for the Warnings section to listing the known standard warnings, or Other if there are no standard warnings. Should uses None if there are none.

I opted to update the status header bc I wanted to allow the actual Warnings list to be anywhere the Code Owners wanted and I think it gives a quick summarization of the component's "risk level", similar to the stability row.

@project-bot project-bot bot added this to In progress in Collector Jun 15, 2022
Collector automation moved this from In progress to Reviewer approved Jun 15, 2022
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

I gave some thought to this after the SIG and I believe that while we can't standardize the 'warnings' entirely there are some similarities between them that could warrant a common classification/glossary/explanation. A few of these that were mentioned at the SIG are:

  • Unsound transformations: incorrect usage of the component may lead to telemetry data that is unsound i.e. not spec-compliant/meaningless. Example components: transform processor
  • Statefulness: the component keeps state related to telemetry data and therefore needs all data from a producer to be sent to the same Collector instance to ensure a correct behavior. Example components: cumulativetodelta processor, any exporter to a backend that does not support cumulative metrics (so e.g. Datadog's or Dynatrace)
  • Identity crisis: the component may change the 'identity' of a metric. Example components: transform, metricstransform

I assume more will come up if we review the currently existing components. We could then have:

  1. A glossary of common caveats with (i) a brief description of the issue and (ii) some sort of identifier (e.g. STATEFUL)
  2. a new section in the README table of each component with a list of the caveats/limitations that apply to it (listed by their identifier?)
  3. A new section in the README table that specifies under which conditions does each caveat/limitation apply and how to use it.

The benefits I see for this approach are:

  1. Component developers can share documentation burden on limitations (e.g. what are common issues that can arise from a given caveat? in which cases should you worry about this?)
  2. End-users have an easy criteria for deciding what components to allow usage of in their infra (e.g. an organization may want to forbid components that have a caveat deemed too risky)

| Stability | [alpha] |
| Supported pipeline types | traces, metrics, logs |
| Distributions | [contrib] |
| Warnings | [Many](#warnings) |
Copy link
Member

@mx-psi mx-psi Jun 15, 2022

Choose a reason for hiding this comment

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

I like the name "caveats" more, although I am not a native English speaker. "Limitations" also sounds like a reasonable name for some of the ones we discussed at the SIG, but maybe it doesn't apply accurately to all of the issues discussed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was purposely trying to avoid the word "caveats" because I thought it might not be good for non-native English speakers. I can switch to Caveats if everything agrees it's ok.

Copy link
Member

Choose a reason for hiding this comment

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

I see! Well 'caveats' is understandable to me in that sense, but I don't want to be the representative of non-native speakers 😄

processor/transformprocessor/README.md Outdated Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Jun 15, 2022

Sorry for the wall of text, I wanted to put my thoughts in writing somewhere :) It may be the case that we don't need to discuss the above on this PR but rather we can keep it for later, so we can also merge this and keep the discussion on a dedicated issue

@TylerHelmuth
Copy link
Member Author

@mx-psi I like the glossary of terms idea as it will allow standard language for everyone to discuss the caveats of a component. I think, though, that there may need to be a combination, because it feels possible that there could be some caveats a code owner wants to call out that are specific to their component.

If everyone likes the glossary idea I think I'd rather handle that now so there is less rework.

@TylerHelmuth
Copy link
Member Author

Pushed a commit with a flavor of the Glossary idea. Ultimately the glossary would live independently in Core. I like how the table now lists out some specific scenarios, but I am kinda worried about the effort to maintain the glossary, mainly how to decide what goes in it.

@mx-psi
Copy link
Member

mx-psi commented Jun 17, 2022

I am kinda worried about the effort to maintain the glossary, mainly how to decide what goes in it.

That's a fair concern. Since we are free to change documentation arbitrarily, if it vaguely feels like a good idea I would say to try it out and see how it works in practice. If it doesn't work out we can just remove it in the future

@TylerHelmuth
Copy link
Member Author

I am kinda worried about the effort to maintain the glossary, mainly how to decide what goes in it.

That's a fair concern. Since we are free to change documentation arbitrarily, if it vaguely feels like a good idea I would say to try it out and see how it works in practice. If it doesn't work out we can just remove it in the future

You're probably right. Adding "Orphaned" felt natural and the more items in the glossary the less likely it is we need to add another.

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 17, 2022
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@TylerHelmuth
Copy link
Member Author

Before this gets merged I need to move the Glossary to some place in Core.

@TylerHelmuth
Copy link
Member Author

@open-telemetry/collector-maintainers, @open-telemetry/collector-approvers, @open-telemetry/collector-contrib-maintainer, @open-telemetry/collector-contrib-approvers, please weigh in.

@TylerHelmuth
Copy link
Member Author

Updated to link to official warnings. Ready for final review.

@mx-psi mx-psi merged commit db18759 into open-telemetry:main Jun 27, 2022
@TylerHelmuth TylerHelmuth deleted the transformprocessor-caveats branch June 27, 2022 17:18
atoulme pushed a commit to atoulme/opentelemetry-collector-contrib that referenced this pull request Jul 16, 2022
* Added warnings sections

* Switched links to use main

* Glossary idea

* Added orphaned to the glossary

* Update processor/transformprocessor/README.md

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>

* Link to standard warnings

* Fix link

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

None yet

3 participants