feature: support standard OTEL exporter protocol environment variables
#63
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent changes to the configuration logic enhance client selection flexibility based on OpenTelemetry exporter protocols. The code now dynamically chooses between gRPC and HTTP clients by checking specific environment variables. This improvement allows users to customize their setup through environment variables, resulting in a more adaptable client configuration. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey @devnev 👋 Thanks you for the PR 👍 I added a few minor comments, feel free to check 😃 |
|
@rustatian do you think its worth putting effort into making this plugin fully compliant with the spec? I can give it a go, but also I think the goal should be to use https://pkg.go.dev/go.opentelemetry.io/contrib/exporters/autoexport directly, but I imagine that can't be done in a non-breaking way. |
|
Maybe a new option |
The case is, that PHP and Go (RR) have separate configurations.. But can be both configured via the same envs. I do think, that we should move closer to this goal (making plugin if not fully, but very close to the spec). |
devnev
force-pushed
the
standard-exporter-protocol-envs
branch
from
83dbe59 to
a582e8e
Compare
I've got a follow up commit to bring more of the plugin inline with the spec (I tried putting it in this PR, but it changes scope too much, I'll raise it as a new PR once this is merged) EDIT: you can see the follow-up changes here: https://github.com/devnev/otel/pull/1/files |
devnev
force-pushed
the
standard-exporter-protocol-envs
branch
from
a582e8e to
27f0cca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- config.go (3 hunks)
Additional comments not posted (1)
config.go (1)
86-86: Enhancement: Dynamic client configuration.The addition of
setClientFromEnvenhances flexibility by allowing the client to be set based on environment variables. Ensure that proper logging is in place for unknown protocols to aid in diagnosing configuration issues.
devnev
force-pushed
the
standard-exporter-protocol-envs
branch
from
27f0cca to
9163291
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- config.go (3 hunks)
Additional comments not posted (2)
config.go (2)
136-139: Good practice: Logging unsupported or unknown protocols.Logging warnings for unsupported or unknown protocols is a good practice as it helps diagnose configuration issues. Ensure that the logs are clear and informative.
84-88: Verify the behavior ofsetClientFromEnv.The function now uses
setClientFromEnvto set the client based on environment variables. Ensure that this function behaves as expected in all scenarios.
|
ping @rustatian |
rustatian
changed the title
OTEL exporter protocol environment variables
No worries, thanks for the review |
Reason for This PR
OpenTelemetry has some standardised environment variables for choosing which client to use: https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
In Go, these can be consumed using the 1st-party package https://pkg.go.dev/go.opentelemetry.io/contrib/exporters/autoexport, but because this otel plugin already does some custom stuff, I've opted to try reimplementing the semantics of the spec in the plugin.
Description of Changes
In case the client is unset in the config, check the standardised environment variables
OTEL_EXPORTER_OTLP_TRACES_PROTOCOLandOTEL_EXPORTER_OTLP_PROTOCOLfor a configured protocol.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes