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

Minmize distro API #941

Merged
merged 13 commits into from
May 24, 2022
Merged

Minmize distro API #941

merged 13 commits into from
May 24, 2022

Conversation

pellared
Copy link
Contributor

@pellared pellared commented May 23, 2022

Why

One or more configuration variables may be needed to properly configure GDI repositories. Components that can be configured with environment variables MUST support configuration of these variables using environment variables. Any component that cannot be configured with environment variables MUST support configuration of these variables using an alternate method. Any component MAY support configuration of these variables by additional methods.

Supporting only a single configuration method is simplifying the codebase and adoption.

From: https://github.com/signalfx/gdi-specification/blob/main/specification/configuration.md

What

  • Minimize github.com/signalfx/splunk-otel-go/distro API to
    contain only necessary option functions.
    • Remove WithAccessToken function,
      use SPLUNK_ACCESS_TOKEN environment variable instead.
    • Remove WithEndpoint function,
      use one of the
      OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, OTEL_EXPORTER_JAEGER_ENDPOINT
      environment variables instead.
    • Remove WithPropagator function,
      use OTEL_PROPAGATORS environment variable instead.
    • Remove WithTraceExporter function,
      use OTEL_TRACES_EXPORTER environment variable instead.
  • Remove most of distro/README.md as I find our official doc in better shape. I plan to create a docs/advanced-config.go based on our official doc in the following days. It should be easier to keep both of them in sync that way and the structure will be similar to our other distros.

Testing

Scenario Trace
default config
SPLUNK_REALM=us0
OTEL_TRACES_EXPORTER=jaeger-thrift-splunk SPLUNK_REALM=us0
OTEL_TRACES_EXPORTER=jaeger-thrift-splunk OTEL_EXPORTER_JAEGER_ENDPOINT=http://localhost:14268/api/traces

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Merging #941 (49be2db) into main (f6edc90) will decrease coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
- Coverage   79.80%   79.31%   -0.49%     
==========================================
  Files          56       56              
  Lines        2832     2780      -52     
==========================================
- Hits         2260     2205      -55     
- Misses        525      527       +2     
- Partials       47       48       +1     
Flag Coverage Δ
Linux 78.95% <100.00%> (-0.50%) ⬇️
Windows 75.44% <100.00%> (-0.62%) ⬇️
macOS 75.21% <100.00%> (-0.57%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
distro/config.go 100.00% <ø> (ø)
distro/exporter.go 95.71% <100.00%> (-4.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6edc90...49be2db. Read the comment docs.

@pellared pellared marked this pull request as ready for review May 23, 2022 12:40
@pellared pellared requested review from a team as code owners May 23, 2022 12:40
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

LGTM. Minor docs concerns only.

CHANGELOG.md Show resolved Hide resolved
distro/exporter.go Outdated Show resolved Hide resolved
distro/exporter.go Outdated Show resolved Hide resolved
pellared and others added 3 commits May 24, 2022 19:34
@pellared pellared enabled auto-merge (squash) May 24, 2022 17:37
@pellared pellared merged commit 7e13b7b into signalfx:main May 24, 2022
@pellared pellared deleted the reduce-api-surface branch May 24, 2022 17:53
This was referenced May 25, 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.

3 participants