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

[pkg/stanza/adapter] Add ability to select one of multiple storage extensions #12871

Closed

Conversation

djaglowski
Copy link
Member

Prior to this change, many log receivers could make use of storage extensions.
However, selecting a storage extension worked as follows:

  1. If exactly one storage extension is configured, use it.
  2. If no storage extensions are configured, do nothing.
  3. If more than one storage extension is configured, fail.

The new behavior depends on a new 'storage' field, which the user can use to
indicate which storgae extension a receiver should use. This works as follows:

  1. If the user does not specify the 'storage' field, behave the same as above.
  2. If the user specifies 'storage: none', do nothing.
  3. If the user specifies 'storage: foo', use 'foo'.
  4. If 'foo' is not configured or is not a storage extension, fail.

Resolves #10915

…tensions

Prior to this change, many log receivers could make use of storage extensions.
However, selecting a storage extension worked as follows:
1. If exactly one storage extension is configured, use it.
2. If no storage extensions are configured, do nothing.
3. If more than one storage extension is configured, fail.

The new behavior depends on a new 'storage' field, which the user can use to
indicate which storgae extension a receiver should use. This works as follows:
1. If the user does not specify the 'storage' field, behave the same as above.
2. If the user specifies 'storage: none', do nothing.
3. If the user specifies 'storage: foo', use 'foo'.
4. If 'foo' is not configured or is not a storage extension, fail.
@djaglowski djaglowski force-pushed the pkg-stanza-storage-name branch from f51def4 to 088f572 Compare August 1, 2022 20:30
@djaglowski djaglowski marked this pull request as ready for review August 2, 2022 13:28
@djaglowski djaglowski requested a review from a team August 2, 2022 13:28
@djaglowski djaglowski requested a review from dmitryax as a code owner August 2, 2022 13:28
@djaglowski djaglowski requested a review from atoulme August 2, 2022 13:28
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

Maybe add a second storage extension in the readme?

@djaglowski
Copy link
Member Author

There's been some additional discussion on the expected behavior over here. New components are not going to use storage extensions by default. Due to this, we need to accept the breaking change (all affected components are still in alpha). I'm moving this PR back to draft status and will make the appropriate updates.

@djaglowski djaglowski marked this pull request as draft August 3, 2022 13:24
}

// Storage explicitly disabled. (e.g. 'storage: false')
if storageID.String() == "false" {
Copy link
Member

Choose a reason for hiding this comment

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

We probably need to add somewhere in the core a check that "false" is not a valid extension type.

Also, should this be "false" or "none" as the PR description suggests?

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Aug 3, 2022

This works as follows:

  1. If the user does not specify the 'storage' field, behave the same as above.
  2. If the user specifies 'storage: none', do nothing.
  3. If the user specifies 'storage: foo', use 'foo'.
  4. If 'foo' is not configured or is not a storage extension, fail.

A further evolution of this could be:
5. If the user specified 'storage: *' then use the first available storage extension.

This can be useful if I need a storage for a component but don't care much about what storage exactly it is, or when I have just one storage extension and don't want to repeat its name everywhere I need to use it.

@djaglowski
Copy link
Member Author

We discussed, in the Collector SIG today, whether or not implicit attachment of an extension is appropriate at all, even though it is the current behavior in this package. All opinions expressed were in support of requiring explicit configuration. The two reasons cited were:

  1. Consistency across the collector. Other components will require explicit configuration of storage. Usage of auth extensions already follow this pattern.
  2. The current implicit behavior is essentially a side effect. The validity of a component's configuration is changed solely based on changes to another component. Similarly, the functionality of a component is directly changed as a side effect of solely adding or removing another component.

@tigrannajaryan, I have changed my mind on this and agree with others that pipeline components should have to explicitly opt in to using storage extensions, even though this would be a breaking change for some (alpha) log receivers. How committed are you to the opinion you expressed about preserving the functionality, either for usability or backwards compatibility?

@tigrannajaryan
Copy link
Member

I agree that explicitly specifying the storage is the right approach.

How committed are you to the opinion you expressed about preserving the functionality, either for usability or backwards compatibility?

Are you referring to my proposal to allow * as the wildcard for storage name? I think this can be useful, but I am fine if we don't want to do it. I wasn't proposing it for backwards compatibility. I think it is OK to break it for an Alpha component.

@dmitryax do you see any problem with this? I think this is used in our Helm Chart, so the chart may need to be updated.

@djaglowski
Copy link
Member Author

Thanks @tigrannajaryan. I was referring to your comments on the related issue here. Sounds like we are on the same page though. 👍

@djaglowski
Copy link
Member Author

Closing in favor of #13418

@djaglowski djaglowski closed this Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support concurrent file storage extensions
4 participants