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

enhancement!: Remove default encoding.codec where appropriate #5281

Merged
merged 18 commits into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference/components/sinks/humio.cue
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ components: sinks: _humio: {
enabled: true
codec: {
enabled: true
default: "json"
default: null
enum: ["json", "text"]
}
}
Expand Down
45 changes: 28 additions & 17 deletions src/sinks/humio/logs.rs
Original file line number Diff line number Diff line change
@@ -1,29 +1,23 @@
use super::{default_host_key, Encoding};
use crate::{
config::{DataType, SinkConfig, SinkContext, SinkDescription},
config::{DataType, GenerateConfig, SinkConfig, SinkContext, SinkDescription},
sinks::splunk_hec::HecSinkConfig,
sinks::util::{
encoding::EncodingConfigWithDefault, BatchConfig, Compression, TowerRequestConfig,
},
sinks::util::{encoding::EncodingConfig, BatchConfig, Compression, TowerRequestConfig},
sinks::{Healthcheck, VectorSink},
template::Template,
};
use serde::{Deserialize, Serialize};

const HOST: &str = "https://cloud.humio.com";

#[derive(Clone, Debug, Deserialize, Serialize, Default)]
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct HumioLogsConfig {
pub(in crate::sinks::humio) token: String,
// Deprecated name
#[serde(alias = "host")]
pub(in crate::sinks::humio) endpoint: Option<String>,
pub(in crate::sinks::humio) source: Option<Template>,
#[serde(
skip_serializing_if = "crate::serde::skip_serializing_if_default",
default
)]
pub(in crate::sinks::humio) encoding: EncodingConfigWithDefault<Encoding>,
pub(in crate::sinks::humio) encoding: EncodingConfig<Encoding>,

pub(in crate::sinks::humio) event_type: Option<Template>,

Expand All @@ -44,7 +38,22 @@ inventory::submit! {
SinkDescription::new::<HumioLogsConfig>("humio_logs")
}

impl_generate_config_from_default!(HumioLogsConfig);
impl GenerateConfig for HumioLogsConfig {
fn generate_config() -> toml::Value {
toml::Value::try_from(Self {
token: "${HUMIO_TOKEN}".to_owned(),
endpoint: None,
source: None,
encoding: Encoding::Json.into(),
event_type: None,
host_key: default_host_key(),
compression: Compression::default(),
request: TowerRequestConfig::default(),
batch: BatchConfig::default(),
})
.unwrap()
}
}

#[async_trait::async_trait]
#[typetag::serde(name = "humio_logs")]
Expand Down Expand Up @@ -74,7 +83,7 @@ impl HumioLogsConfig {
index: None,
sourcetype: self.event_type.clone(),
source: self.source.clone(),
encoding: self.encoding.clone().without_default(),
encoding: self.encoding.clone().into_encoding(),
compression: self.compression,
batch: self.batch,
request: self.request,
Expand Down Expand Up @@ -262,17 +271,19 @@ mod integration_tests {

/// create a new test config with the given ingest token
fn config(token: &str) -> super::HumioLogsConfig {
super::HumioLogsConfig {
endpoint: Some(HOST.to_string()),
HumioLogsConfig {
token: token.to_string(),
compression: Compression::None,
endpoint: Some(HOST.to_string()),
source: None,
encoding: Encoding::Json.into(),
event_type: None,
host_key: log_schema().host_key().to_string(),
compression: Compression::None,
request: TowerRequestConfig::default(),
batch: BatchConfig {
max_events: Some(1),
..Default::default()
},
host_key: log_schema().host_key().to_string(),
..Default::default()
}
}

Expand Down
10 changes: 2 additions & 8 deletions src/sinks/humio/metrics.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use super::{default_host_key, logs::HumioLogsConfig, Encoding};
use crate::{
config::{DataType, GenerateConfig, SinkConfig, SinkContext, SinkDescription, TransformConfig},
sinks::util::{
encoding::EncodingConfigWithDefault, BatchConfig, Compression, TowerRequestConfig,
},
sinks::util::{encoding::EncodingConfig, BatchConfig, Compression, TowerRequestConfig},
sinks::{Healthcheck, VectorSink},
template::Template,
transforms::metric_to_log::MetricToLogConfig,
Expand All @@ -21,11 +19,7 @@ pub struct HumioMetricsConfig {
#[serde(alias = "host")]
pub(in crate::sinks::humio) endpoint: Option<String>,
source: Option<Template>,
#[serde(
skip_serializing_if = "crate::serde::skip_serializing_if_default",
default
)]
encoding: EncodingConfigWithDefault<Encoding>,
encoding: EncodingConfig<Encoding>,

event_type: Option<Template>,

Expand Down
4 changes: 1 addition & 3 deletions src/sinks/humio/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@ pub mod metrics;
use crate::sinks::splunk_hec;
use serde::{Deserialize, Serialize};

#[derive(Deserialize, Serialize, Debug, Eq, PartialEq, Clone, Derivative)]
#[derive(Deserialize, Serialize, Debug, Eq, PartialEq, Clone)]
#[serde(rename_all = "snake_case")]
#[derivative(Default)]
pub enum Encoding {
#[derivative(Default)]
Json,
Text,
}
Expand Down
48 changes: 29 additions & 19 deletions src/sinks/new_relic_logs.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use crate::{
config::{DataType, SinkConfig, SinkContext, SinkDescription},
config::{DataType, GenerateConfig, SinkConfig, SinkContext, SinkDescription},
sinks::{
http::{HttpMethod, HttpSinkConfig},
util::{
encoding::EncodingConfigWithDefault, BatchConfig, Compression, Concurrency,
TowerRequestConfig,
encoding::EncodingConfig, BatchConfig, Compression, Concurrency, TowerRequestConfig,
},
},
};
Expand Down Expand Up @@ -38,17 +37,12 @@ pub enum NewRelicLogsRegion {
Eu,
}

#[derive(Deserialize, Serialize, Debug, Derivative, Clone)]
#[derivative(Default)]
#[derive(Deserialize, Serialize, Debug, Clone)]
pub struct NewRelicLogsConfig {
pub license_key: Option<String>,
pub insert_key: Option<String>,
pub region: Option<NewRelicLogsRegion>,
#[serde(
default,
skip_serializing_if = "crate::serde::skip_serializing_if_default"
)]
pub encoding: EncodingConfigWithDefault<Encoding>,
pub encoding: EncodingConfig<Encoding>,
#[serde(default)]
pub compression: Compression,
#[serde(default)]
Expand All @@ -62,13 +56,15 @@ inventory::submit! {
SinkDescription::new::<NewRelicLogsConfig>("new_relic_logs")
}

impl_generate_config_from_default!(NewRelicLogsConfig);
impl GenerateConfig for NewRelicLogsConfig {
fn generate_config() -> toml::Value {
toml::Value::try_from(Self::with_encoding(Encoding::Json)).unwrap()
}
}

#[derive(Deserialize, Serialize, Debug, Eq, PartialEq, Clone, Derivative)]
#[derive(Deserialize, Serialize, Debug, Eq, PartialEq, Clone)]
#[serde(rename_all = "snake_case")]
#[derivative(Default)]
pub enum Encoding {
#[derivative(Default)]
Json,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add Text here as an encoding option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you okay with adding in another PR? This will not be breaking change because we already removed Default for Encoding.

Copy link
Member

Choose a reason for hiding this comment

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

👍 a separate PR is fine, but it'd be nice to do it before this change is released in a new version as it'd be confusing to require users to specify the encoding.codec option when there is only one valid value.

}

Expand Down Expand Up @@ -101,6 +97,18 @@ impl SinkConfig for NewRelicLogsConfig {
}

impl NewRelicLogsConfig {
fn with_encoding(encoding: Encoding) -> Self {
Self {
license_key: None,
insert_key: None,
region: None,
encoding: encoding.into(),
compression: Compression::default(),
batch: BatchConfig::default(),
request: TowerRequestConfig::default(),
}
}

fn create_config(&self) -> crate::Result<HttpSinkConfig> {
let mut headers: IndexMap<String, String> = IndexMap::new();

Expand Down Expand Up @@ -143,7 +151,7 @@ impl NewRelicLogsConfig {
auth: None,
headers: Some(headers),
compression: self.compression,
encoding: self.encoding.clone().without_default(),
encoding: self.encoding.clone().into_encoding(),

batch,
request,
Expand Down Expand Up @@ -178,7 +186,9 @@ mod tests {
assert_eq!(
format!(
"{}",
NewRelicLogsConfig::default().create_config().unwrap_err()
NewRelicLogsConfig::with_encoding(Encoding::Json)
.create_config()
.unwrap_err()
),
"Missing authentication key, must provide either 'license_key' or 'insert_key'"
.to_owned(),
Expand All @@ -187,7 +197,7 @@ mod tests {

#[test]
fn new_relic_logs_check_config_defaults() {
let mut nr_config = NewRelicLogsConfig::default();
let mut nr_config = NewRelicLogsConfig::with_encoding(Encoding::Json);
nr_config.license_key = Some("foo".to_owned());
let http_config = nr_config.create_config().unwrap();

Expand All @@ -210,7 +220,7 @@ mod tests {

#[test]
fn new_relic_logs_check_config_custom() {
let mut nr_config = NewRelicLogsConfig::default();
let mut nr_config = NewRelicLogsConfig::with_encoding(Encoding::Json);
nr_config.insert_key = Some("foo".to_owned());
nr_config.region = Some(NewRelicLogsRegion::Eu);
nr_config.batch.max_size = Some(MAX_PAYLOAD_SIZE);
Expand Down Expand Up @@ -293,7 +303,7 @@ mod tests {
async fn new_relic_logs_happy_path() {
let in_addr = next_addr();

let mut nr_config = NewRelicLogsConfig::default();
let mut nr_config = NewRelicLogsConfig::with_encoding(Encoding::Json);
nr_config.license_key = Some("foo".to_owned());
let mut http_config = nr_config.create_config().unwrap();
http_config.uri = format!("http://{}/fake_nr", in_addr)
Expand Down
16 changes: 16 additions & 0 deletions src/sinks/util/encoding/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,22 @@ where
}
}

#[cfg(any(feature = "sinks-new_relic_logs", feature = "sinks-humio"))]
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused why this type only is used by these two sinks and not the others. Should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's should be here. Only used in New Relic / Humio because we need to convert EncodingConfig<sink1::Encoding> to EncodingConfig<sink2::Encoding>.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Gotcha, I see.

impl<E> EncodingConfig<E> {
pub(crate) fn into_encoding<X>(self) -> EncodingConfig<X>
where
X: From<E>,
{
EncodingConfig {
codec: self.codec.into(),
schema: self.schema,
only_fields: self.only_fields,
except_fields: self.except_fields,
timestamp_format: self.timestamp_format,
}
}
}

impl<E> From<E> for EncodingConfig<E> {
fn from(codec: E) -> Self {
Self {
Expand Down
19 changes: 0 additions & 19 deletions src/sinks/util/encoding/with_default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,25 +55,6 @@ impl<E: Default + PartialEq> EncodingConfiguration<E> for EncodingConfigWithDefa
}
}

#[cfg(any(feature = "sinks-new_relic_logs", feature = "sinks-humio"))]
impl<E> EncodingConfigWithDefault<E>
where
E: Default + PartialEq,
{
pub(crate) fn without_default<X>(self) -> crate::sinks::util::encoding::EncodingConfig<X>
where
X: From<E> + PartialEq,
{
crate::sinks::util::encoding::EncodingConfig {
codec: self.codec.into(),
schema: self.schema,
only_fields: self.only_fields,
except_fields: self.except_fields,
timestamp_format: self.timestamp_format,
}
}
}

impl<E> From<E> for EncodingConfigWithDefault<E>
where
E: Default + PartialEq,
Expand Down