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.

Sign up for GitHub

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

Jump to bottom

Standardize on params vs settings vs options #2650

Closed
bogdandrutu opened this issue Mar 10, 2021 · 6 comments · Fixed by #3163, #3164, #3167, #3181 or #3294
Closed

Standardize on params vs settings vs options #2650

bogdandrutu opened this issue Mar 10, 2021 · 6 comments · Fixed by #3163, #3164, #3167, #3181 or #3294
Assignees
@pmatyjasek-sumo
Labels
area:miscellaneous help wanted Good issue for contributors to OpenTelemetry Service to pick up release:required-for-ga Must be resolved before GA release
Milestone
Phase1-Trace-GA-R...

Comments

@bogdandrutu
Copy link
Member

In few places for constructors we use "Params" (in the components), some "Parameters" (in the Service) or "Settings" in the components helpers like exporterhelper.

We need to decide on a name a pattern: expose a struct with these properties or expose "Options" and use variadic args.

Consolidate all interfaces to follow the same pattern.
@bogdandrutu bogdandrutu added help wanted Good issue for contributors to OpenTelemetry Service to pick up area:miscellaneous labels Mar 10, 2021
@bogdandrutu bogdandrutu added this to the Phase1-GA-Roadmap milestone Mar 10, 2021
@bogdandrutu bogdandrutu changed the title Standardize on params vs settings Standardize on params vs settings vs options Mar 10, 2021
@naseemkullah
Copy link
Member

How about Config ? That is the term used by Zap: https://github.com/uber-go/zap/blob/89e382035d3a984a01e6c9c8be5462f11efac844/config.go#L58

@bogdandrutu
Copy link
Member Author

@naseemkullah thanks for the suggestion, we use Settings the most, and probably to limit the amount of breaking changes we should stick with that.

Maybe we can look how many times we use each of them and make a decision. Config also has a problem because in our project Config is the file yaml config that we use to configure the entire service, and may be confusing for users.

@pmatyjasek-sumo
Copy link
Contributor

Is someone working on this issue? If not I can do that

@tigrannajaryan
Copy link
Member

Thanks @pmatyjasek-sumo, assigned to you.

pmatyjasek-sumo added a commit to pmatyjasek-sumo/opentelemetry-collector that referenced this issue Apr 22, 2021
@pmatyjasek-sumo
Standarize Settings instead of Params and Parameters
b033941 
Fixes open-telemetry#2650
Changes:
 Replace: ReceiverCreateParams, ProcessorCreateParams, ExtensionCreateParams and ExporterCreateParams with ComponentSettings struct
 Replace all dependencies in Extensions, Exporters, Processors and Receivers
 Replace Parameters and settings structs with new struct Settings
 Replace dependencies in service and main
 Update tests

Signed-off-by: Patryk Matyjasek <[email protected]>
@pmatyjasek-sumo pmatyjasek-sumo mentioned this issue Apr 22, 2021
Closed
pmatyjasek-sumo added a commit to pmatyjasek-sumo/opentelemetry-collector that referenced this issue Apr 23, 2021
@pmatyjasek-sumo
Standarize Settings instead of Params and Parameters
ee1079e 
Fixes open-telemetry#2650
Changes:
 Replace: ReceiverCreateParams, ProcessorCreateParams, ExtensionCreateParams and ExporterCreateParams with ComponentSettings struct
 Replace all dependencies in Extensions, Exporters, Processors and Receivers
 Replace Parameters and settings structs with new struct Settings
 Replace dependencies in service and main
 Update tests

Signed-off-by: Patryk Matyjasek <[email protected]>

# Conflicts:
#	exporter/kafkaexporter/factory.go
#	exporter/kafkaexporter/factory_test.go
#	exporter/kafkaexporter/kafka_exporter.go
#	exporter/kafkaexporter/kafka_exporter_test.go
#	exporter/prometheusexporter/factory.go
#	receiver/jaegerreceiver/jaeger_agent_test.go
#	receiver/jaegerreceiver/trace_receiver_test.go
#	receiver/kafkareceiver/factory.go
#	receiver/kafkareceiver/factory_test.go
#	receiver/kafkareceiver/kafka_receiver.go
#	receiver/kafkareceiver/kafka_receiver_test.go
pmatyjasek-sumo added a commit to pmatyjasek-sumo/opentelemetry-collector that referenced this issue Apr 28, 2021
@pmatyjasek-sumo
Standarize Settings instead of Params and Parameters
e603bd8 
Fixes open-telemetry#2650
Changes:
 Replace: ReceiverCreateParams, ProcessorCreateParams, ExtensionCreateParams and ExporterCreateParams with ComponentSettings struct
 Replace all dependencies in Extensions, Exporters, Processors and Receivers
 Replace Parameters and settings structs with new struct Settings
 Replace dependencies in service and main
 Update tests

Signed-off-by: Patryk Matyjasek <[email protected]>

# Conflicts:
#	exporter/kafkaexporter/factory.go
#	exporter/kafkaexporter/factory_test.go
#	exporter/kafkaexporter/kafka_exporter.go
#	exporter/kafkaexporter/kafka_exporter_test.go
#	exporter/prometheusexporter/factory.go
#	receiver/jaegerreceiver/jaeger_agent_test.go
#	receiver/jaegerreceiver/trace_receiver_test.go
#	receiver/kafkareceiver/factory.go
#	receiver/kafkareceiver/factory_test.go
#	receiver/kafkareceiver/kafka_receiver.go
#	receiver/kafkareceiver/kafka_receiver_test.go

# Conflicts:
#	processor/batchprocessor/batch_processor.go
#	processor/batchprocessor/batch_processor_test.go
#	processor/batchprocessor/factory.go
#	processor/filterprocessor/filter_processor_test.go
#	service/application.go
@pmatyjasek-sumo pmatyjasek-sumo mentioned this issue Apr 28, 2021
Closed
pmatyjasek-sumo added a commit to pmatyjasek-sumo/opentelemetry-collector that referenced this issue May 5, 2021
@pmatyjasek-sumo
Standarize Settings instead of Params and Parameters
e18dd72 
Fixes open-telemetry#2650
Changes:
 Replace: ReceiverCreateParams, ProcessorCreateParams, ExtensionCreateParams and ExporterCreateParams with ComponentSettings struct
 Replace all dependencies in Extensions, Exporters, Processors and Receivers
 Replace Parameters and settings structs with new struct Settings
 Replace dependencies in service and main
 Update tests

Signed-off-by: Patryk Matyjasek <[email protected]>

# Conflicts:
#	exporter/kafkaexporter/factory.go
#	exporter/kafkaexporter/factory_test.go
#	exporter/kafkaexporter/kafka_exporter.go
#	exporter/kafkaexporter/kafka_exporter_test.go
#	exporter/prometheusexporter/factory.go
#	receiver/jaegerreceiver/jaeger_agent_test.go
#	receiver/jaegerreceiver/trace_receiver_test.go
#	receiver/kafkareceiver/factory.go
#	receiver/kafkareceiver/factory_test.go
#	receiver/kafkareceiver/kafka_receiver.go
#	receiver/kafkareceiver/kafka_receiver_test.go

# Conflicts:
#	processor/batchprocessor/batch_processor.go
#	processor/batchprocessor/batch_processor_test.go
#	processor/batchprocessor/factory.go
#	processor/filterprocessor/filter_processor_test.go
#	service/application.go

# Conflicts:
#	cmd/otelcol/main.go
#	component/component.go
#	component/exporter.go
#	component/extension.go
#	component/processor.go
#	component/processor_test.go
#	component/receiver.go
#	exporter/exporterhelper/factory_test.go
#	extension/extensionhelper/factory_test.go
#	processor/batchprocessor/batch_processor.go
#	processor/processorhelper/factory_test.go
#	receiver/jaegerreceiver/factory.go
#	receiver/jaegerreceiver/trace_receiver_test.go
#	receiver/otlpreceiver/factory.go
#	receiver/prometheusreceiver/factory.go
#	receiver/receiverhelper/factory_test.go
#	service/application.go
#	service/application_test.go
#	service/defaultcomponents/default_exporters_test.go
#	service/defaultcomponents/default_extensions_test.go
#	service/defaultcomponents/default_processors_test.go
#	service/defaultcomponents/default_receivers_test.go
#	service/internal/builder/exporters_builder.go
#	service/internal/builder/extensions_builder.go
#	service/internal/builder/pipelines_builder.go
#	service/internal/builder/receivers_builder.go
#	service/service.go
#	service/service_test.go
#	testbed/testbed/otelcol_runner.go
@alolita
Copy link
Member

alolita commented May 12, 2021

@mxiamxia @anuraaga @Aneurysm9 can you please review?

@anuraaga
Copy link
Contributor

@alolita As far as I can tell we're waiting on splitting out #2991 into smaller chunks

This was referenced May 13, 2021
Merged
Merged
Merged
Merged
@alolita alolita added the release:required-for-ga Must be resolved before GA release label May 18, 2021
@pmatyjasek-sumo pmatyjasek-sumo mentioned this issue May 25, 2021
Merged
@bogdandrutu bogdandrutu reopened this May 26, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this issue Apr 27, 2023
@atoulme
Changes to the pulsarexporter configuration to prepare with using ups…
ceaae86 
…tream (open-telemetry#2650)

* Changes to the pulsarexporter configuration to prepare with using upstream

* fieldalignment fix
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this issue Jul 5, 2024
@marcalff
[BUILD] Upgrade bazel abseil from 20220623.1 to 20230802.2 (open-tele…
0ca7d70 
…metry#2650)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pmatyjasek-sumo pmatyjasek-sumo

Labels
area:miscellaneous help wanted Good issue for contributors to OpenTelemetry Service to pick up release:required-for-ga Must be resolved before GA release
Projects
None yet
Milestone
Phase1-Trace-GA-Roadmap
6 participants
@anuraaga @bogdandrutu @alolita @tigrannajaryan @naseemkullah @pmatyjasek-sumo