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

Indicate in component readme if the component is stateful or stateless #18878

Closed
atoulme opened this issue Feb 22, 2023 · 16 comments
Closed

Indicate in component readme if the component is stateful or stateless #18878

atoulme opened this issue Feb 22, 2023 · 16 comments
Labels
discussion needed Community discussion needed documentation Improvements or additions to documentation enhancement New feature or request

Comments

@atoulme
Copy link
Contributor

atoulme commented Feb 22, 2023

Component(s)

No response

Is your feature request related to a problem? Please describe.

We want to document if a component is stateful ir stareless to help users make the best decision when scaling up the collector.

Describe the solution you'd like

I propose adding a state row to components README files indicating if they are stateless or stateful.

we also can define what stateless and stateful means in our docs or even at the specification level.

Describe alternatives you've considered

No response

Additional context

No response

@atoulme atoulme added enhancement New feature or request needs triage New item requiring triage labels Feb 22, 2023
@mx-psi
Copy link
Member

mx-psi commented Feb 23, 2023

We already have a definition of statefulness in our docs on the standard warnings list and some components (not many) have this in a separate row example.

I think we can use this issue to track adding that row for other components.

@atoulme atoulme added documentation Improvements or additions to documentation and removed needs triage New item requiring triage labels Feb 24, 2023
@atoulme
Copy link
Contributor Author

atoulme commented Feb 24, 2023

This "Warnings" row seems to explicitly call out some risks of using the component, but is being statless or stateful something to warn about?

@aurbiztondo-splunk
Copy link
Contributor

Thanks for the tip, @mx-psi. I have a few thoughts/questions:

  • I think this deserves a proper definition somewhere, not just the warning. I can draft some definitions and place them somewhere TBD. I'm still getting acquainted with the repo so any suggestions are welcome. We should also explain the YAML syntax.
  • Is a component always either stateless or stateful, or does the state depend on the specific instance?
    ** If the "state-ness" doesn't change, then I'd add it to the table at the beginning of each component's README.
    ** If it does change, then it should be mentioned somewhere.
  • Does the component state only impact scaling, or does it have more implications?

@mx-psi
Copy link
Member

mx-psi commented Feb 24, 2023

This "Warnings" row seems to explicitly call out some risks of using the component, but is being statless or stateful something to warn about?

Maybe 'Warnings' is too strong of a word, but the intention was to list properties of a component that require special attention/restrict the kinds of deployments where it can be used. Statefulness usually means e.g. you have to ensure a given producer of telemetry data needs to send all their data to the same Collector instance, so it does require special attention and thus fits in there with the other 'warnings'. If you think there's a better name for this or that we should make a distinction between different kinds of warnings that would be great to improve, I am just trying to point out the existing work on this area.

I think this deserves a proper definition somewhere, not just the warning. I can draft some definitions and place them somewhere TBD. I'm still getting acquainted with the repo so any suggestions are welcome.

We can iterate on the existing definition on the 'Statefulness' subsection here and spell out the implications more clearly.

  • Is a component always either stateless or stateful, or does the state depend on the specific instance?
    ** If the "state-ness" doesn't change, then I'd add it to the table at the beginning of each component's README.
    ** If it does change, then it should be mentioned somewhere.
  • Does the component state only impact scaling, or does it have more implications?

Agreed that these are important, having notes about how a 'warning' applies to a component makes sense to me.


cc @TylerHelmuth who may have opinions on this :)

@TylerHelmuth
Copy link
Member

is being stateless or stateful something to warn about?

I would argue Statefulness is something to Warn about. Statefulness in a component has a huge impact on how you architect your collector deployments. If you use a Stateful component without understanding how it impacts your use of the collector you will most likely end up with flawed data.

But the dangerous part of a stateful component is that it can't tell you that you're using it wrong. If you use the cumulativetodeltaprocessor in deployment with multiple pods and metrics end up on different pods it will never tell you that the math its performing isn't accurate anymore. Similarly if you use the tailsamplingprocessor and send spans to a different collectors the processor won't know that its missing spans. Instead in both situations you'll have to catch the issue at the place your exporting the data, which could be difficult.

I think the Warning section is the appropriate place to handle this. You can see what it is like in the transform processor and cumulativetodelta processor. Both use a format that gives information at a quick glance but also allow for more context if the user is inclined.

Here are the 2 PRs where this worked started:

If Warnings is too strong we had started with Caveats but switched. I like the boldness of "Warning", though, as I implies that you can seriously hurt yourself if you're not careful.

@jpkrohling
Copy link
Member

I'm for the status quo, I quite like "warnings" here for the same reason @TylerHelmuth mentioned above.

@atoulme
Copy link
Contributor Author

atoulme commented Feb 28, 2023

The issue I have with "Warnings" is that we wouldn't warn that a component is stateless. I would want to document that a component is stateless explicitly rather than just document that some are stateful to make it more uniform and also provide a strong signal to readers that committers have thought through this and want to signal this component is usable as stateless.

If you don't say it's stateful or stateless, then I as a reader would make the assumption the developer has not spent the time to identify that characteristic.

@TylerHelmuth
Copy link
Member

@atoulme thats fair, the danger is definitely with Stateful, but there are considerations for Stateless that I think are fair to warn about as well (although caveat is a nicer word when discussing Stateless). For example, a component like the metricstransformprocessor is explicitly Stateless at the moment, so it doesn't take into account metrics across batches. As a result, it can't be used to aggregate metrics from multiple sources, like across multiple nodes/clients. This is important distinction for users to consider when deciding to use the component. In fairness, metricstransformprocess might be the only Contrib processor that has this important Stateless distinction.

I have definitely been operating under the assumption that components are Stateless by default. Hence the warning in the transformprocess does not have a Stateless warning. We could split out State into its own block, but I tend to vote more for "Stateless is default, safe, and allows for scaling" and "Stateful is explicitly, and has deployment and scaling implications".

@jpkrohling
Copy link
Member

but I tend to vote more for "Stateless is default, safe, and allows for scaling" and "Stateful is explicitly, and has deployment and scaling implications"

+1

@atoulme
Copy link
Contributor Author

atoulme commented Feb 28, 2023

OK. Can I negotiate something here? I’d like that beta components display the warning row as a requirement for beta accession, and make sure owners label state expectations then. So if nothing is listed, it’s default stateless and someone looked into it for sure as part of beta requirements.

@TylerHelmuth
Copy link
Member

@atoulme I think that works. Are you free to bring this discussion to the SIG meeting tomorrow?

@atoulme
Copy link
Contributor Author

atoulme commented Feb 28, 2023

I will be there.

@atoulme atoulme added the discussion needed Community discussion needed label Feb 28, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 1, 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
Copy link
Contributor

github-actions bot commented Jul 3, 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 Jul 3, 2023
@mx-psi
Copy link
Member

mx-psi commented Jul 3, 2023

Should we close this in favor of #19172 ?

@github-actions github-actions bot removed the Stale label Jul 4, 2023
@TylerHelmuth
Copy link
Member

I believe we can.

@mx-psi mx-psi closed this as completed Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Community discussion needed documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants