Upgrade for "Use configoptional.Optional for exporterhelper QueueBatchConfig" #44320
Conversation
jmacd
changed the title
jmacd
requested review from
a team,
ArthurSens,
MovieStoreGuy,
TylerHelmuth,
andrzej-stencel,
atoulme,
axw,
bogdandrutu,
braydonk,
crobert-1,
dashpole,
dmitryax,
mx-psi and
songy23
as code owners
github-actions
bot
added
exporter/prometheus
exporter/otelarrow
exporter/opensearch
exporter/mezmo
exporter/logzio
exporter/logicmonitor
exporter/loadbalancing
exporter/kafka
exporter/influxdb
exporter/honeycombmarker
exporter/googlemanagedprometheus
|
|
||
| replace github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/ecsutil => ../../internal/aws/ecsutil | ||
|
|
||
| replace go.opentelemetry.io/config/configoptional => /home/jmacd/src/otel/opentelemetry-collector/config/configoptional |
|
|
||
| // Wrap async implementation of sendMetricsAsync into a sync-callable API. | ||
| s.sync2Async = internal.NewSync2Async(s.set.Logger, s.cfg.QueueConfig.NumConsumers, s.sendMetricsAsync) | ||
| s.sync2Async = internal.NewSync2Async(s.set.Logger, s.cfg.QueueConfig.Get().NumConsumers, s.sendMetricsAsync) |
There was a problem hiding this comment.
Get() will return nil if the config is unset, right? So does this need changing to prevent a nil dereference panic?
| } | ||
| if cfg.QueueBatchConfig.Batch.HasValue() { | ||
| qbCfg := cfg.QueueBatchConfig.Batch.Get() | ||
| fmt.Println("DANGER DANGER") |
There was a problem hiding this comment.
| fmt.Println("DANGER DANGER") |
high voltage!
| qbCfg.MaxSize = int64(cfg.Flush.Bytes) | ||
| } | ||
| } | ||
| fmt.Println("END (DANGER DANGER)") |
There was a problem hiding this comment.
| fmt.Println("END (DANGER DANGER)") |
| cm, err := confmaptest.LoadConf(filepath.Join("testdata", tt.configFile)) | ||
| require.NoError(t, err) | ||
|
|
||
| fmt.Println("START HERE") |
There was a problem hiding this comment.
| fmt.Println("START HERE") |
| fmt.Println("START HERE") | ||
| sub, err := cm.Sub(tt.id.String()) | ||
| require.NoError(t, err) | ||
| fmt.Println("ONE CALL") |
There was a problem hiding this comment.
| fmt.Println("ONE CALL") |
| require.NoError(t, err) | ||
| fmt.Println("ONE CALL") | ||
| require.NoError(t, sub.Unmarshal(cfg)) | ||
| fmt.Println("FINISH") |
There was a problem hiding this comment.
| fmt.Println("FINISH") |
|
|
||
| fmt.Println("HEY", cfg.(*Config).QueueBatchConfig.HasValue()) | ||
| fmt.Println("BATCH", cfg.(*Config).QueueBatchConfig.HasValue()) | ||
|
|
There was a problem hiding this comment.
| fmt.Println("HEY", cfg.(*Config).QueueBatchConfig.HasValue()) | |
| fmt.Println("BATCH", cfg.(*Config).QueueBatchConfig.HasValue()) |
MovieStoreGuy
left a comment
MovieStoreGuy
left a comment
There was a problem hiding this comment.
I am not sure I understand why there some types are being marked as optional like QueueSettings, when there is always a default being applied?
If this configuration type hinting for users, wouldn't struct tags be easier to adopt and less of a breaking change?
We want to have a consistent way of enabling and disabling an optional section, there were, for the most part, two mechanisms (otlp receiver protocols section and enabled), we are trying to be consistent by representing this in the same way everywhere
Struct tags do not allow for custom unmarshaling behavior which is necessary for some use cases of configoptional |
5 participants
Description
Aims to fix the contrib build following open-telemetry/opentelemetry-collector#14155.