-
Notifications
You must be signed in to change notification settings - Fork 1.7k
chore(config): Update field labels for commonly used sources and transforms #17517
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
Changes from 3 commits
0caf194
c1bb582
2555b20
73349f7
95bc065
bee219d
b1fa963
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,26 +107,30 @@ pub struct KafkaSourceConfig { | |
#[configurable(metadata(docs::examples = 5000, docs::examples = 10000))] | ||
#[configurable(metadata(docs::advanced))] | ||
#[serde(default = "default_session_timeout_ms")] | ||
#[configurable(metadata(docs::human_name = "Session Timeout"))] | ||
session_timeout_ms: Duration, | ||
|
||
/// Timeout for network requests. | ||
#[serde_as(as = "serde_with::DurationMilliSeconds<u64>")] | ||
#[configurable(metadata(docs::examples = 30000, docs::examples = 60000))] | ||
#[configurable(metadata(docs::advanced))] | ||
#[serde(default = "default_socket_timeout_ms")] | ||
#[configurable(metadata(docs::human_name = "Socket Timeout"))] | ||
socket_timeout_ms: Duration, | ||
|
||
/// Maximum time the broker may wait to fill the response. | ||
#[serde_as(as = "serde_with::DurationMilliSeconds<u64>")] | ||
#[configurable(metadata(docs::examples = 50, docs::examples = 100))] | ||
#[configurable(metadata(docs::advanced))] | ||
#[serde(default = "default_fetch_wait_max_ms")] | ||
#[configurable(metadata(docs::human_name = "Fetch Wait Max"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be better written as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, updated. |
||
fetch_wait_max_ms: Duration, | ||
|
||
/// The frequency that the consumer offsets are committed (written) to offset storage. | ||
#[serde_as(as = "serde_with::DurationMilliSeconds<u64>")] | ||
#[serde(default = "default_commit_interval_ms")] | ||
#[configurable(metadata(docs::examples = 5000, docs::examples = 10000))] | ||
#[configurable(metadata(docs::human_name = "Commit Interval"))] | ||
commit_interval_ms: Duration, | ||
|
||
/// Overrides the name of the log field used to add the message key to each event. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ pub struct OpentelemetryConfig { | |
#[configurable(metadata(docs::examples = "example_grpc_config()"))] | ||
#[derive(Clone, Debug)] | ||
#[serde(deny_unknown_fields)] | ||
#[configurable(metadata(docs::human_name = "gRPC"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be added to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yes, and actually, it's already there. I'll remove this addition. |
||
struct GrpcConfig { | ||
/// The socket address to listen for connections on. | ||
/// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,7 @@ pub struct RemapConfig { | |
/// | ||
/// [vrl_docs_abort]: https://vector.dev/docs/reference/vrl/expressions/#abort | ||
#[serde(default = "crate::serde::default_true")] | ||
#[configurable(metadata(docs::human_name = "Drop on Abort"))] | ||
pub drop_on_abort: bool, | ||
|
||
/// Reroutes dropped events to a named output instead of halting processing on them. | ||
|
@@ -129,6 +130,7 @@ pub struct RemapConfig { | |
/// to a specially-named output, `dropped`. The original event is annotated with additional | ||
/// fields describing why the event was dropped. | ||
#[serde(default = "crate::serde::default_false")] | ||
#[configurable(metadata(docs::human_name = "Drop on Error"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I think this line should be up at https://github.com/vectordotdev/vector/pull/17517/files#diff-ad5088e389a2b5ae3b514738658923d21b23dda2de8b29cfb4022c73cd0f8bd2L105 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @silversupreme @spencergilbert I think I finally got this right. I used |
||
pub reroute_dropped: bool, | ||
|
||
#[configurable(derived, metadata(docs::hidden))] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's in scope for this PR, but I would support putting a
docs::hidden
metadata here to hide this in the config builder. We shouldn't be showing deprecated options like this to users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would also drop it from the Vector docs - which might not be desired until we actually remove it. That said, it's probably been deprecated for more than a release and can be removed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a list of fields that are noted as deprecated. This is one of them, so I'm thinking we can address them all in one PR?