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

Small clarification for default's service.name for exporters. #1386

Merged
merged 7 commits into from
Jan 29, 2021
Merged

Small clarification for default's service.name for exporters. #1386

merged 7 commits into from
Jan 29, 2021

Conversation

carlosalberto
Copy link
Contributor

Fixes #1237
Fixes #1380

Small clarification on using the default Resource is no service.name was used.

PS - Don't think we need a CHANGELOG entry for this, but happy to add one if needed.

@carlosalberto carlosalberto requested a review from a team as a code owner January 28, 2021 15:51
@carlosalberto carlosalberto requested a review from a team January 28, 2021 15:51
@carlosalberto carlosalberto requested a review from a team as a code owner January 28, 2021 15:51
Oberon00
Oberon00 reviewed Jan 28, 2021
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Nit: "was specified" sounds a bit like there may be another way beyond resources to specify a service name.

specification/trace/sdk_exporters/jaeger.md Outdated Show resolved Hide resolved
specification/trace/sdk_exporters/zipkin.md Outdated Show resolved Hide resolved
carlosalberto and others added 2 commits January 28, 2021 17:38
@carlosalberto @Oberon00
Update specification/trace/sdk_exporters/jaeger.md
4114f7f 
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@carlosalberto @Oberon00
Update specification/trace/sdk_exporters/zipkin.md
fc67837 
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
andrewhsu
andrewhsu approved these changes Jan 28, 2021
View reviewed changes
Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas
Copy link
Member

PS - Don't think we need a CHANGELOG entry for this, but happy to add one if needed

I'd add it, as this is easy to overlook by language SIGs. A changelog would make it more easy.

iNikem
iNikem reviewed Jan 29, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@carlosalberto @iNikem
Update CHANGELOG.md
fa20e52 
Co-authored-by: Nikita Salnikov-Tarnovski <gnikem@gmail.com>
@carlosalberto carlosalberto merged commit 37e9991 into open-telemetry:main Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem left review comments

@andrewhsu andrewhsu andrewhsu approved these changes

@codeboten codeboten codeboten approved these changes

@Oberon00 Oberon00 Oberon00 approved these changes

@jkwatson jkwatson jkwatson approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@cijothomas cijothomas cijothomas approved these changes

@arminru arminru arminru approved these changes

Assignees

@bogdandrutu bogdandrutu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Zipkin spec should suggest to use Resource.GetDefault Jaeger exporter service.name clarification needed
10 participants
@carlosalberto @cijothomas @andrewhsu @codeboten @Oberon00 @jkwatson @iNikem @yurishkuro @arminru @bogdandrutu