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.

Sign up for GitHub

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

Jump to bottom

Add environment variables for Jaeger exporter #796

Merged
merged 11 commits into from
Jun 10, 2020
Merged

Add environment variables for Jaeger exporter #796

merged 11 commits into from
Jun 10, 2020

Conversation

XSAM
Copy link
Member

@XSAM XSAM commented Jun 5, 2020

Resolves #792

Handle the following environment variables:

  • JAEGER_SERVICE_NAME
  • JAEGER_DISABLED
  • JAEGER_TAGS
  • JAEGER_ENDPOINT
  • JAEGER_USER
  • JAEGER_PASSWORD

@XSAM
Copy link
Member Author

XSAM commented Jun 5, 2020

It looks like TestPushTicker fails the CI.

@Aneurysm9
Copy link
Member

I like the use of Options to pull in the environment data, but I think they should be specified as default options so that a user doesn't need to do any configuration beyond setting the environment variables. That's my recollection of how the config with OpenTracing worked and I think it would provide the best developer experience.

@XSAM XSAM force-pushed the feature/jaeger-exporter-env branch from 7a7bdf5 to a2e8905 Compare June 6, 2020 08:53
@XSAM
Copy link
Member Author

XSAM commented Jun 6, 2020

These options now specified as default options.

@XSAM XSAM force-pushed the feature/jaeger-exporter-env branch from 53a14cd to 1d58f63 Compare June 8, 2020 09:20
@evantorrie evantorrie closed this Jun 8, 2020
@XSAM
Copy link
Member Author

XSAM commented Jun 8, 2020

@evantorrie Why has this PR been closed?

evantorrie
evantorrie reviewed Jun 8, 2020
View reviewed changes
internal/testing/env.go Show resolved Hide resolved
internal/testing/env.go Outdated Show resolved Hide resolved
internal/testing/env.go Outdated Show resolved Hide resolved
@evantorrie
Copy link
Contributor

@evantorrie Why has this PR been closed?

Sorry that was a mistake! Let me reopen it.

@evantorrie evantorrie reopened this Jun 8, 2020
This was referenced Jun 8, 2020
Closed
Closed
@XSAM XSAM force-pushed the feature/jaeger-exporter-env branch from 1d58f63 to 5f9f780 Compare June 9, 2020 00:14
evantorrie
evantorrie approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@evantorrie evantorrie left a comment

Choose a reason for hiding this comment

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

+1 from me!

MrAlias
MrAlias reviewed Jun 9, 2020
View reviewed changes
exporters/trace/jaeger/env.go Outdated Show resolved Hide resolved
exporters/trace/jaeger/env.go Show resolved Hide resolved
exporters/trace/jaeger/env.go Show resolved Hide resolved
exporters/trace/jaeger/jaeger.go Outdated Show resolved Hide resolved
XSAM added 3 commits June 10, 2020 11:10
@XSAM
Add disabled field for jaeger exporter
a234027
@XSAM
Add environment variables for jaeger exporter
d8d7b41 
Handle these environment variables: JAEGER_SERVICE_NAME, JAEGER_DISABLED, JAEGER_TAGS, JAEGER_ENDPOINT, JAEGER_USER, JAEGER_PASSWORD
@XSAM
Add license header
11032a8
@XSAM XSAM force-pushed the feature/jaeger-exporter-env branch from 5f9f780 to 659aac3 Compare June 10, 2020 09:19
@MrAlias MrAlias merged commit 4bf35c6 into open-telemetry:master Jun 10, 2020
@XSAM XSAM deleted the feature/jaeger-exporter-env branch June 11, 2020 00:53
@XSAM XSAM mentioned this pull request Aug 1, 2020
Closed
6 tasks
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Aug 28, 2020
@MrAlias
Add @XSAM as project approver
8e7f58d 
[Requirements](https://github.com/open-telemetry/community/blob/master/community-membership.md#requirements-2)
- [X] >=10 substantive contributions (open-telemetry/opentelemetry-go-contrib#134, open-telemetry/opentelemetry-go-contrib#153, open-telemetry/opentelemetry-go-contrib#221, open-telemetry/opentelemetry-go-contrib#298, open-telemetry/opentelemetry-go-contrib#303, open-telemetry#796, open-telemetry#905, open-telemetry#986, open-telemetry#1044, open-telemetry#1031, open-telemetry#1102)
- [X] Active >1mo
- [X] add to CODEOWNERS  (done in this pull)
- [X] Add to CONTRIBUTING.md (done in this pull)
- [X] Maintainer nomination: @MrAlias
- [ ] Other maintainer(s) (@Aneurysm9) sign-off
- [ ]  Add to @open-telemetry/go-approvers
This was referenced Aug 28, 2020
Merged
Merged
@MrAlias MrAlias mentioned this pull request Apr 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@jmacd jmacd jmacd approved these changes

@MrAlias MrAlias MrAlias approved these changes

@evantorrie evantorrie evantorrie approved these changes

@lizthegrey lizthegrey Awaiting requested review from lizthegrey

@paivagustavo paivagustavo Awaiting requested review from paivagustavo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Jaeger exporter should honor environment variables
5 participants
@XSAM @Aneurysm9 @evantorrie @jmacd @MrAlias