-
Notifications
You must be signed in to change notification settings - Fork 2.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
Support sampling startegies file flag in OTEL collector #2195
Support sampling startegies file flag in OTEL collector #2195
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2195 +/- ##
==========================================
+ Coverage 96.16% 96.18% +0.01%
==========================================
Files 219 219
Lines 10632 10632
==========================================
+ Hits 10224 10226 +2
+ Misses 352 351 -1
+ Partials 56 55 -1
Continue to review full report at Codecov.
|
6c3b192
to
f8094e3
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
f8094e3
to
cad205f
Compare
I'm assuming we are only talking about the custom Jaeger build of the OTel Collector? If so, can't we remove the default values and have them moved to the default OTel Collector config? So then, the flags are only override values. |
cmd/opentelemetry-collector/app/receiver/jaegerreceiver/jaeger_receiver.go
Outdated
Show resolved
Hide resolved
cmd/opentelemetry-collector/app/receiver/jaegerreceiver/jaeger_receiver.go
Outdated
Show resolved
Hide resolved
cmd/opentelemetry-collector/app/receiver/jaegerreceiver/jaeger_receiver_test.go
Show resolved
Hide resolved
@@ -104,6 +107,7 @@ func main() { | |||
// getOTELConfigFile returns name of OTEL config file. | |||
func getOTELConfigFile() string { | |||
f := &flag.FlagSet{} | |||
f.SetOutput(ioutil.Discard) |
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.
What is the reason for adding this?
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.
This can print out an error in some situations. getOTELConfigFile
is a hack that parses flags before actual parsing by cobra command so it's fine to ignore it. If there is an error it is recognized once the command runs.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
PR rebased and updated |
Resolves #2154
This patch showcases how upstream OTEL components can be extended to support additional means of configuration. In this case I have added
sampling.strategies-file
flag to Jaeger receiver. The configuration in OTEL config has higher precedence over the configuration in Viper (like we do for other jaeger flags).sampling.strategies-file
is used and the sampling strategies in the OTEL config is missing it would enable it. e.g.:Now let's talk about how this approach could work for other flags. Flags that don't have a default value can be supported easily. However flags with a default value e.g.
--collector.http-server.host-port=:14268
are problematic. The problem is that when the default config is created we don't know whether this endpoint should be enabled or disabled as there is always a default value. When the collector is started without a configuration file it's not a problem because we enable all Jaeger endpoints. However when the collector is started with a configuration file the default value in the flag always enables the endpoint - which might be problematic if users want to disable the endpoint.