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

Update SpanProcessor Shutdown with context and error #1264

Merged
merged 11 commits into from
Oct 27, 2020
Merged

Update SpanProcessor Shutdown with context and error #1264

merged 11 commits into from
Oct 27, 2020

Conversation

ilyakaznacheev
Copy link
Contributor

@ilyakaznacheev ilyakaznacheev commented Oct 18, 2020 •
edited
Loading

Closes #1232

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 18, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@ilyakaznacheev ilyakaznacheev changed the title [WIP] Update SpanProcessor Shutdown with context and error Update SpanProcessor Shutdown with context and error Oct 18, 2020
@codecov
Copy link

codecov bot commented Oct 18, 2020 •
edited
Loading

Codecov Report

Merging #1264 into master will decrease coverage by 0.0%.
The diff coverage is 86.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1264     +/-   ##
========================================
- Coverage    76.9%   76.8%   -0.1%     
========================================
  Files         122     122             
  Lines        5934    5944     +10     
========================================
+ Hits         4564    4568      +4     
- Misses       1122    1129      +7     
+ Partials      248     247      -1
Impacted Files Coverage Δ
sdk/trace/batch_span_processor.go 74.5% <83.3%> (-4.0%) ⬇️
sdk/trace/provider.go 94.4% <100.0%> (ø)
sdk/trace/simple_span_processor.go 71.4% <100.0%> (+4.7%) ⬆️

@ilyakaznacheev
Copy link
Contributor Author

I failed with CI a bit, so sorry for that 😔

XSAM
XSAM reviewed Oct 23, 2020
View reviewed changes
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

Looks good. All comments are for minor questions. ;)

sdk/trace/provider.go Outdated Show resolved Hide resolved
example/otel-collector/main.go Outdated Show resolved Hide resolved
sdk/trace/batch_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/batch_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/batch_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/simple_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/span_processor_test.go Outdated Show resolved Hide resolved
XSAM
XSAM approved these changes Oct 24, 2020
View reviewed changes
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

LGTM

@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package priority:p1 labels Oct 27, 2020
@MrAlias MrAlias added this to the RC1 milestone Oct 27, 2020
@MrAlias MrAlias merged commit a6b31e0 into open-telemetry:master Oct 27, 2020
samm02 added a commit to samm02/opentelemetry-go that referenced this pull request Oct 27, 2020
@samm02
Merge pull request #1 from open-telemetry/master
0b7d00c 
Update SpanProcessor Shutdown with context and error (open-telemetry#1264)
ilyakaznacheev
ilyakaznacheev commented Oct 27, 2020
View reviewed changes
example/basic/main.go Outdated Show resolved Hide resolved
@samm02 samm02 mentioned this pull request Oct 29, 2020
Closed
AzfaarQureshi pushed a commit to open-o11y/opentelemetry-go that referenced this pull request Dec 3, 2020
@ilyakaznacheev @MrAlias
Update SpanProcessor Shutdown with context and error (open-telemetry#…
91ef4d8 
…1264)

* 1232: update SpanProcessor Shutdown with context and error

* 1232: add changelog info

* 1232: fix CI error, rm commented code

* 1232: fix CI unhandled error

* 1232: Done commit properly

* Add shutdown error handling

* Merge branch 'master' into update-span-processor

* Revert now unneeded context declaration move

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <codingalias@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrAlias MrAlias MrAlias approved these changes

@XSAM XSAM XSAM approved these changes

@dashpole dashpole dashpole approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@evantorrie evantorrie Awaiting requested review from evantorrie

@jmacd jmacd Awaiting requested review from jmacd

@lizthegrey lizthegrey Awaiting requested review from lizthegrey

Assignees
No one assigned
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Projects
None yet
Milestone
RC1
Development

Successfully merging this pull request may close these issues.

Update the SpanProcessor Shutdown method
5 participants
@ilyakaznacheev @Aneurysm9 @dashpole @MrAlias @XSAM