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

Refactor OTLP exporter env config to be shared across all exporters #2608

Merged
merged 7 commits into from
Mar 3, 2022
Merged

Refactor OTLP exporter env config to be shared across all exporters #2608

merged 7 commits into from
Mar 3, 2022

Conversation

dmathieu
Copy link
Member

The envconfig from both OTLP exporters is highly repeated code.
As we want to remove the gRPC dependency from HTTP packages, we would need to duplicate even more.

So this is a proposal to make it somewhat reusable.
There is still some code repetition, as it wouldn't be trivial to extract GenericOption into a shared package. However, I believe this moves in the right direction, as quite a bit of logic is now being shared.

@dmathieu
Copy link
Member Author

This doesn't introduce any changes to public APIs. So I didn't add a changelog entry.

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #2608 (347b8ce) into main (18f4cb8) will decrease coverage by 0.0%.
The diff coverage is 98.1%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2608     +/-   ##
=======================================
- Coverage   75.6%   75.6%   -0.1%     
=======================================
  Files        171     172      +1     
  Lines      11629   11551     -78     
=======================================
- Hits        8801    8736     -65     
+ Misses      2615    2605     -10     
+ Partials     213     210      -3     
Impacted Files Coverage Δ
exporters/otlp/internal/envconfig/envconfig.go 95.5% <95.5%> (ø)
...s/otlp/otlpmetric/internal/otlpconfig/envconfig.go 100.0% <100.0%> (+4.8%) ⬆️
...rs/otlp/otlptrace/internal/otlpconfig/envconfig.go 100.0% <100.0%> (+4.8%) ⬆️
sdk/trace/batch_span_processor.go 82.1% <0.0%> (+0.9%) ⬆️

@pellared pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 14, 2022
@pellared
Copy link
Member

I just made a quick look and the approach LGTM.

@Aneurysm9
Copy link
Member

This doesn't introduce any changes to public APIs. So I didn't add a changelog entry.

I still think this should have a changelog entry. It may not change any public APIs, but it may change behavior and having a changelog entry may help users to know that there was a change in this area that may be related to any issues they experience.

Skipping the changelog entry should be for things like documentation changes or test-only changes.

@Aneurysm9 Aneurysm9 removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 14, 2022
@pellared
Copy link
Member

it may change behavior

@dmathieu Is it so? Could you please explain in the PR description? It looks like pure refactoring which should be invisible to the user.

@dmathieu
Copy link
Member Author

No, this shouldn't change the behavior in any way a user of the exporter would notice. The API remains the same, and environment variables also keep the same behavior.
There can always be unseen regressions obviously.

@pellared
Copy link
Member

Skipping the changelog entry should be for things like documentation changes or test-only changes.

@Aneurysm9 As a user, I find internal refactoring changelog entries as noise. Still, up to you. I respect your decisions 👍

@Aneurysm9
Copy link
Member

this shouldn't change the behavior in any way a user of the exporter would notice. The API remains the same, and environment variables also keep the same behavior.
There can always be unseen regressions obviously.

This is the core of my point. While no behavior change is intended, that is different from no behavior change being possible. Without an indication in the changelog that a change was made to this exporter a user who is experiencing a regression may be misled and delayed in identifying this change as a potential cause.

Copy link
Member

@hanyuancheung hanyuancheung left a comment

Choose a reason for hiding this comment

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

LGTM

exporters/otlp/internal/envconfig/envconfig.go Outdated Show resolved Hide resolved
@Aneurysm9 Aneurysm9 merged commit 24414b2 into open-telemetry:main Mar 3, 2022
@dmathieu dmathieu deleted the envconfig branch March 3, 2022 08:15
hanyuancheung added a commit to hanyuancheung/opentelemetry-go that referenced this pull request Mar 5, 2022
…pen-telemetry#2608)

* setup global envconfig package for otlp exporter

* use envconfig in otlpmetrics package

* fix lint

* add changelog entry

* Update exporters/otlp/internal/envconfig/envconfig.go

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>

* fix lint

Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@MrAlias MrAlias added this to the Release v1.5.0 milestone Mar 15, 2022
@MrAlias MrAlias mentioned this pull request Mar 15, 2022
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.

5 participants