Skip to content

Standarize Settings, Params and Parameters in Exporters#3164

Merged
bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
pmatyjasek-sumo:pm_standarize_settings_exporters
Jun 2, 2021
Merged

Standarize Settings, Params and Parameters in Exporters#3164
bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
pmatyjasek-sumo:pm_standarize_settings_exporters

Conversation

@pmatyjasek-sumo
Copy link
Copy Markdown
Contributor

@pmatyjasek-sumo pmatyjasek-sumo commented May 13, 2021

Description:
Continuation work from #2991
Fixes #2650
Replace ExporterCreateParams with ExporterCreateSettings.
Replace all dependencies in Exporters.

Link to tracking Issue:
#2650

Testing:
Update unit tests

Documentation:
In code comments

@pmatyjasek-sumo pmatyjasek-sumo requested a review from a team May 13, 2021 11:29
assert.Equal(t, &nopExporterConfig{ExporterSettings: config.NewExporterSettings(config.NewID("nop"))}, cfg)

traces, err := factory.CreateTracesExporter(context.Background(), component.ExporterCreateParams{}, cfg)
traces, err := factory.CreateTracesExporter(context.Background(), component.ComponentSettings{}, cfg)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is used a lot in tests (lot's of time with zap.Nop), probably good to have in componenttest a NopComponentSettings or TestComponentSettings that uses zap.Nop + DefaultBuildInfo and pass that in tests.

@github-actions
Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 21, 2021
@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_exporters branch 2 times, most recently from e48d8ab to 8006b6e Compare May 24, 2021 08:44
@bogdandrutu bogdandrutu removed the Stale label May 24, 2021
@carlosalberto
Copy link
Copy Markdown
Contributor

Looks good to me, but needs to fix to apply @bogdandrutu's feedback, resolve conflicts and make sure the tests pass.

@bogdandrutu
Copy link
Copy Markdown
Member

Please rebase

@bogdandrutu
Copy link
Copy Markdown
Member

Also add a changelog entry

@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_exporters branch from 8006b6e to 31aebd6 Compare May 27, 2021 12:48
@pmatyjasek-sumo pmatyjasek-sumo requested a review from alolita as a code owner May 27, 2021 12:48
@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_exporters branch from 31aebd6 to 9677096 Compare May 31, 2021 08:58
@bogdandrutu
Copy link
Copy Markdown
Member

Please rebase the PR to fix merge conflicts

Replace all dependencies in Exporters.

Signed-off-by: Patryk Matyjasek <pmatyjasek@sumologic.com>

# Conflicts:
#	exporter/prometheusremotewriteexporter/factory.go

# Conflicts:
#	exporter/fileexporter/factory.go
#	exporter/opencensusexporter/factory_test.go
#	exporter/otlpexporter/factory_test.go
#	exporter/prometheusremotewriteexporter/factory_test.go

# Conflicts:
#	CHANGELOG.md
#	exporter/kafkaexporter/factory.go
#	exporter/opencensusexporter/factory.go
#	exporter/otlpexporter/factory.go
#	exporter/otlphttpexporter/factory.go
#	exporter/zipkinexporter/factory.go

# Conflicts:
#	CHANGELOG.md
#	component/exporter.go
#	exporter/opencensusexporter/factory_test.go
@pmatyjasek-sumo pmatyjasek-sumo force-pushed the pm_standarize_settings_exporters branch from 9677096 to 8e5a1a0 Compare June 2, 2021 12:17
@pmatyjasek-sumo
Copy link
Copy Markdown
Contributor Author

rebase done

@bogdandrutu bogdandrutu merged commit 1f7ffab into open-telemetry:main Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardize on params vs settings vs options

3 participants