-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Improve OC Receiver Documentation #203
Changes from all commits
96211b4
4a0c43f
c6b11e0
54e334c
bad4b8b
7b1d161
048068a
c8d8d89
dbfbc7f
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 |
---|---|---|
|
@@ -55,6 +55,8 @@ const ( | |
errPipelineExporterNotExists | ||
errMetricPipelineCannotHaveProcessors | ||
errUnmarshalError | ||
errMissingReceivers | ||
ccaraman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// TODO(ccaraman): Add an error for missing Processors with corresponding test cases. | ||
) | ||
|
||
type configError struct { | ||
|
@@ -175,6 +177,19 @@ func loadReceivers(v *viper.Viper, factories map[string]receiver.Factory) (confi | |
// Get the map of "receivers" sub-keys. | ||
keyMap := v.GetStringMap(receiversKeyName) | ||
|
||
// Currently there is no default receiver enabled. The configuration must specify at least one receiver to enable | ||
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. We have separate validation code. Would it make sense to move this code there? Also, I would suggest to split this PR into 2 parts: code change and documentation change. It is difficult to review both at the same time. 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. I forgot to add a todo to the validateReceivers that we need to revisit the approach of deleting the disabled receivers because it sounds like there will be a cli way to enable/disable a receiver. One reason why the validateReceivers need to happen later on in the process of loading a configuration is loading pipelines checks if a receiver is enabled or not before adding it to its configuration so we can't remove the disabled receivers in this part. 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. Re splitting this pr into 2 -are you okay with doing it for future prs to keep the history of the conversations? 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. I highly recommend to split the PR. You do not need to delete this one, just remove changes that are not relevant to documenting existing behavior. One PR and one commit per concern is much nicer. This PR is performing 2 mostly independent changes, I do not see good reasons to group them. I'd just remove config validation code changes from this PR and keep everything about documentation and tests which are related to documentation. Let this be a separate PR:
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. Synced offline - This will be a one off and leave all of the changes together since i ended up fixing code functionality that was related to fixing the docs (ex: ensuring that there is one enabled receiver was mentioned but not enforced in code with no test case either) |
||
// functionality. | ||
if len(keyMap) == 0 { | ||
return nil, &configError{ | ||
code: errMissingReceivers, | ||
msg: "no receivers specified in config", | ||
} | ||
} | ||
|
||
// This boolean is used to track if there are any enabled receivers. If there are none at the end of loading | ||
// all of the receivers, throw an error. | ||
enabledReceiver := false | ||
|
||
// Prepare resulting map | ||
receivers := make(configmodels.Receivers, 0) | ||
|
||
|
@@ -211,6 +226,9 @@ func loadReceivers(v *viper.Viper, factories map[string]receiver.Factory) (confi | |
err = customUnmarshaler(subViper, key, receiverCfg) | ||
} else { | ||
// Standard viper unmarshaler is fine. | ||
// TODO(ccaraman): UnmarshallExact should be used to catch erroneous config entries. | ||
// This leads to quickly identifying config values that are not supported and reduce confusion for | ||
// users. | ||
err = subViper.UnmarshalKey(key, receiverCfg) | ||
} | ||
|
||
|
@@ -228,9 +246,20 @@ func loadReceivers(v *viper.Viper, factories map[string]receiver.Factory) (confi | |
} | ||
} | ||
|
||
// Or'ing the enabled flag for all receivers will return true if at least one is enabled. | ||
enabledReceiver = enabledReceiver || receiverCfg.IsEnabled() | ||
|
||
receivers[fullName] = receiverCfg | ||
} | ||
|
||
// There must be at least one enabled receiver for the config to be valid. | ||
if !enabledReceiver { | ||
return nil, &configError{ | ||
code: errMissingReceivers, | ||
msg: "no enabled receivers specified in config", | ||
} | ||
} | ||
|
||
return receivers, nil | ||
} | ||
|
||
|
@@ -496,6 +525,7 @@ func validatePipelineReceivers( | |
zap.String("receiver", ref)) | ||
} | ||
} | ||
|
||
pipeline.Receivers = rs | ||
|
||
return nil | ||
|
@@ -594,6 +624,9 @@ func validatePipelineProcessors( | |
return nil | ||
} | ||
|
||
// TODO(ccaraman): Determine if there a way to consolidate the validate receivers apart of the loadReceivers method. | ||
// Currently, validateReceivers needs to be invoked after validatePipelineReceivers because that bit of | ||
// code checks if a receiver is enabled prior to finalizing the pipelines. | ||
func validateReceivers(cfg *configmodels.Config) { | ||
// Remove disabled receivers. | ||
for name, rcv := range cfg.Receivers { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,7 +176,8 @@ func TestDecodeConfig_Invalid(t *testing.T) { | |
}{ | ||
{name: "empty-config"}, | ||
{name: "missing-all-sections"}, | ||
{name: "missing-receivers"}, | ||
{name: "missing-enabled-receivers", expected: errMissingReceivers}, | ||
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. 👍 |
||
{name: "missing-receivers", expected: errMissingReceivers}, | ||
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. 👍 |
||
{name: "missing-exporters"}, | ||
{name: "missing-processors"}, | ||
{name: "invalid-receiver-name"}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,11 +142,19 @@ const ( | |
// ReceiverSettings defines common settings for a single-protocol receiver configuration. | ||
// Specific receivers can embed this struct and extend it with more fields if needed. | ||
type ReceiverSettings struct { | ||
TypeVal string `mapstructure:"-"` | ||
NameVal string `mapstructure:"-"` | ||
Disabled bool `mapstructure:"disabled"` | ||
Endpoint string `mapstructure:"endpoint"` | ||
DisableBackPressure bool `mapstructure:"disable-backpressure"` | ||
TypeVal string `mapstructure:"-"` | ||
NameVal string `mapstructure:"-"` | ||
// Configures if the receiver is disabled and doesn't receive any data. | ||
// The default value is false(meaning the receiver is enabled by default), and it is expected that receivers | ||
// continue to use the default value of false. | ||
Disabled bool `mapstructure:"disabled"` | ||
// Configures the endpoint in the format 'address:port' for the receiver. | ||
// The default value is set by the receiver populating the struct. | ||
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. 👍 |
||
Endpoint string `mapstructure:"endpoint"` | ||
// Configures if the back pressure functionality is disabled for this receiver. | ||
// The default value is false, and it is expected that receivers | ||
// continue to use the default value of false. | ||
DisableBackPressure bool `mapstructure:"disable-backpressure"` | ||
} | ||
|
||
// Name gets the receiver name. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
receivers: | ||
examplereceiver: | ||
disabled: true | ||
examplereceiver/disabled: | ||
disabled: true | ||
|
||
processors: | ||
exampleprocessor: | ||
exampleprocessor/disabled: | ||
disabled: true | ||
|
||
exporters: | ||
exampleexporter/myexporter: | ||
extra: "some export string 2" | ||
exampleexporter/disabled: | ||
disabled: true | ||
exampleexporter: | ||
|
||
pipelines: | ||
traces: | ||
receivers: [examplereceiver, examplereceiver/disabled] | ||
processors: [exampleprocessor, exampleprocessor/disabled] | ||
exporters: [exampleexporter/disabled, exampleexporter] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Pipelines | ||
*Note* This documentation is still in progress. For any questions, please reach | ||
out in the [OpenTelemetry Gitter](https://gitter.im/open-telemetry/opentelemetry-service) | ||
or refer to the [issues page](https://github.com/open-telemetry/opentelemetry-service/issues). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Processors | ||
*Note* This documentation is still in progress. For any questions, please reach | ||
out in the [OpenTelemetry Gitter](https://gitter.im/open-telemetry/opentelemetry-service) | ||
or refer to the [issues page](https://github.com/open-telemetry/opentelemetry-service/issues). |
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.
👍