docs(rfc): split component build lifecycle into four phases#25848
docs(rfc): split component build lifecycle into four phases#25848pront wants to merge 2 commits into
Conversation
github-actions
Bot
added
domain: ci
This comment has been minimized.
This comment has been minimized.
pront
added
the
no-changelog
7563720 to
8ea1964
Compare
github-actions
Bot
removed
the
domain: ci
8ea1964 to
9d53fd9
Compare
pront
changed the title
pront
changed the title
Proposes a shared ComponentConfig trait with validate_structure, validate_environment, build, and start phases for TransformConfig and SinkConfig, deferring SourceConfig to future work.
9d53fd9 to
e59f2fe
Compare
pront
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e59f2fe827
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| | Call site | Phases invoked (transforms) | Phases invoked (sinks) | | ||
| | --- | --- | --- | | ||
| | `vector validate --no-environment` | `validate_structure` | `validate_structure` | |
There was a problem hiding this comment.
For vector validate --no-environment, this row would only invoke validate_structure, but current validation deliberately runs transform validation before the no_environment gate (src/validate.rs:170-174) and chains validate_env() for every transform (src/validate.rs:284-290). Following this matrix would skip VRL/condition compilation again for --no-environment, contradicting the stated no-user-visible-change goal and reopening the gap this RFC cites as the immediate motivation; either this path needs transform validate_environment with stubs/no I/O, or those checks need to be classified as structural.
Useful? React with 👍 / 👎.
| /// Phase 3: pure, synchronous construction. Receives context; produces `Self::Built`. | ||
| /// No task spawning, no I/O. Safe to discard on topology rollback. | ||
| fn build(&self, cx: &Self::Context) -> crate::Result<Self::Built>; |
There was a problem hiding this comment.
This makes build() synchronous and gives validate_environment only a Result, but some sinks need awaited setup artifacts to construct the sink itself, not just to run healthchecks: checked src/sinks/aws_s3/config.rs:232/:366-375 (create_service(...).await) and HTTP AWS auth in src/sinks/http/config.rs:371-383 (credentials_provider(...).await?). Since the RFC later rejects returning artifacts from validation, normal startup would have no way to construct these sinks under this trait; the proposal needs either async build or a validation artifact that build can consume.
Useful? React with 👍 / 👎.
|
|
||
| | Call site | Phases invoked (transforms) | Phases invoked (sinks) | | ||
| | --- | --- | --- | | ||
| | `vector validate --no-environment` | `validate_structure` | `validate_structure` | |
There was a problem hiding this comment.
This row leaves sinks at validate_structure only under vector validate --no-environment, so the split still does not exercise the pure sink construction checks that the motivation calls out as currently skipped when build() is tied to healthchecks. Current validation only enters the component/healthcheck path when no_environment is false (src/validate.rs:172-174); after healthchecks are separated, this path needs to call build or an equivalent construction-validation phase for sinks, otherwise auth, encoder, and template checks left in pure construction remain skipped.
Useful? React with 👍 / 👎.
| 1. Add `ComponentConfig` with `validate_structure`, `validate_environment`, and sync `build` as | ||
| required methods, with a blanket adapter that delegates all three to each trait's existing async | ||
| `build()` for un-migrated components. |
There was a problem hiding this comment.
The blanket adapter cannot safely delegate validate_structure to the existing async build(): combined with the no-environment call matrix, every un-migrated transform or sink would run the legacy build path during vector validate --no-environment, which is the side-effectful path this RFC is trying to split. I checked existing transforms such as aws_ec2_metadata, whose build refreshes metadata and then spawns a spawn_timed worker (src/transforms/aws_ec2_metadata.rs:226-247), so the adapter needs a side-effect-free/default structure implementation and should not call legacy build except on environment/startup paths.
Useful? React with 👍 / 👎.
| impl Transform { | ||
| /// Phase 4: startup. Spawns background tasks, opens connections, registers metrics. | ||
| /// Called only after the topology diff is committed. | ||
| async fn start(self, cx: &TransformContext) -> RunningTransform { ... } |
There was a problem hiding this comment.
As written, Transform::start(self, cx: &TransformContext) has no input receiver or output controls. Current startup constructs the transform task with input_rx and TransformOutputs (src/topology/builder.rs:824-845) and registers the returned controls before spawning (src/topology/builder.rs:621-627); implementing this signature would leave post-commit start unable to wire events through the transform. The phase split needs a start API that receives or returns these topology channels, or it should keep task construction in TopologyPiecesBuilder.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds an RFC proposing a shared
ComponentConfigtrait that splits the monolithicbuild()into four explicit lifecycle phases:validate_structure,validate_environment,build(pure/sync), andstart.Motivation:
build()currently conflates structural validation, environment validation, pure construction, and task spawning, which breaksvector validate --no-environmentand leaks background tasks on topology reload rollback.Scope:
TransformConfigandSinkConfig.SourceConfigis deferred to future work.Vector configuration
N/A -- no behavior change.
How did you test this PR?
RFC only -- no code changes.
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.