Skip to content

Change exporterhelper to accept ExporterCreateSettings instead of just logger#3569

Merged
bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
bogdandrutu:exporterhelperset
Jul 8, 2021
Merged

Change exporterhelper to accept ExporterCreateSettings instead of just logger#3569
bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
bogdandrutu:exporterhelperset

Conversation

@bogdandrutu
Copy link
Copy Markdown
Member

In a future PR ExporterCreateSettings will carry Tracer and Meter for custom tracing/metrics.

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

…t logger

In a future PR ExporterCreateSettings will carry Tracer and Meter for custom tracing/metrics.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu requested a review from alolita as a code owner July 6, 2021 00:14
@bogdandrutu bogdandrutu requested review from a team and tigrannajaryan July 6, 2021 00:14
Copy link
Copy Markdown
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Should we have ExporterHelperSettings instead and only pass what is needed by the helper and not entire ExporterCreateSettings? Passing ExporterCreateSettings seems to unnecessarily couple the exporter and helper stronger than necessary.

Not a strong opinion, just thinking out loud.

@bogdandrutu
Copy link
Copy Markdown
Member Author

Passing ExporterCreateSettings seems to unnecessarily couple the exporter and helper stronger than necessary.

I do see this, but passing the entire ExporterCreateSettings is more future proof, since adding a new element in the ExporterCreateSettings as an example I want to add the Tracer there will make all callers to automatically pass that to the helper without any other change, similar at one point we will pass Meter.

@bogdandrutu bogdandrutu merged commit 7f5bff2 into open-telemetry:main Jul 8, 2021
@bogdandrutu bogdandrutu deleted the exporterhelperset branch July 8, 2021 17:57
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.

2 participants