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.

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

autoexport: Change WithFallback options signatures #4877

Closed
pellared opened this issue Jan 31, 2024 · 2 comments · Fixed by #4891
Closed

autoexport: Change WithFallback options signatures #4877

pellared opened this issue Jan 31, 2024 · 2 comments · Fixed by #4891
Assignees
Labels
area: exporter Related to an exporter package enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@pellared
Copy link
Member

pellared commented Jan 31, 2024

Problem Statement

The current definition of fallback options is as following:

func WithFallbackMetricReader(exporter metric.Reader) MetricOption

The problem is that we need to create an instance of a metric reader or span exporter and it may not even be needed/used (it may even leak resources).

Proposed Solution

Change the signature to accept a factory function e.g.

func WithFallbackMetricReader(factory func(context.Context) (metric.Reader, error)) MetricOption
@pellared pellared added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed area: exporter Related to an exporter package labels Jan 31, 2024
@MrAlias

This comment was marked as resolved.

hcelaloner added a commit to hcelaloner/opentelemetry-go-contrib that referenced this issue Feb 5, 2024
Fixes open-telemetry#4877. Changes the method signatures as requested in the issue.
@hcelaloner
Copy link
Contributor

Hi @pellared, I went ahead and prepared a PR for the requested change: #4891

I directly changed the signature of the method as requested. Doesn't this create a breaking change for existing autoexport users who use that method? How do you proceed in that kind of situation in opentelemetry-go-contrib? Should we create a new method and deprecate the old one instead?

hcelaloner added a commit to hcelaloner/opentelemetry-go-contrib that referenced this issue Feb 5, 2024
Fixes open-telemetry#4877. Changes the method signatures as requested in the issue.
MrAlias added a commit that referenced this issue Feb 6, 2024
* feat(autoexport): change WithFallback options signatures

Fixes #4877. Changes the method signatures as requested in the issue.

* chore: apply changelog suggestion

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* chore(autoexport): get rid hasFallback flag which became extra after fallbackFactory change

---------

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias MrAlias added this to the v1.23.0/v0.48.0/v0.17.0/v0.3.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: exporter Related to an exporter package enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants