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

fix: unref jaeger socket to prevent process running indefinitely #715

Merged
merged 4 commits into from
Jan 24, 2020
Merged

fix: unref jaeger socket to prevent process running indefinitely #715

merged 4 commits into from
Jan 24, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jan 21, 2020

Fixes #712

@dyladan dyladan added the bug Something isn't working label Jan 21, 2020
@codecov-io
Copy link

codecov-io commented Jan 21, 2020 •
edited
Loading

Codecov Report

Merging #715 into master will decrease coverage by 1.98%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   92.94%   90.95%   -1.99%     
==========================================
  Files         225      226       +1     
  Lines       10786    10359     -427     
  Branches      977      955      -22     
==========================================
- Hits        10025     9422     -603     
- Misses        761      937     +176
Impacted Files Coverage Δ
...s/opentelemetry-tracing/src/BasicTracerRegistry.ts 100% <ø> (ø) ⬆️
...lemetry-core/test/trace/globaltracer-utils.test.ts 96.96% <ø> (ø) ⬆️
...-metrics/test/export/ConsoleMetricExporter.test.ts 100% <ø> (ø) ⬆️
...pentelemetry-core/src/metrics/NoopMeterRegistry.ts 100% <100%> (ø)
.../opentelemetry-core/test/metrics/NoopMeter.test.ts 97.29% <100%> (+0.07%) ⬆️
...ckages/opentelemetry-plugin-ioredis/src/ioredis.ts 100% <100%> (ø) ⬆️
...y-core/src/context/propagation/HttpTraceContext.ts 100% <100%> (ø) ⬆️
...emetry-exporter-prometheus/test/prometheus.test.ts 98.6% <100%> (ø) ⬆️
packages/opentelemetry-tracing/src/Tracer.ts 98.38% <100%> (-0.03%) ⬇️
...ackages/opentelemetry-metrics/src/MeterRegistry.ts 100% <100%> (ø)
... and 58 more

OlivierAlbertini
OlivierAlbertini approved these changes Jan 21, 2020
View reviewed changes
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

💯

packages/opentelemetry-exporter-jaeger/src/jaeger.ts
@@ -40,6 +41,11 @@ export class JaegerExporter implements SpanExporter {
typeof config.flushTimeout === 'number' ? config.flushTimeout : 2000;

this._sender = new jaegerTypes.UDPSender(config);
if (this._sender._client instanceof Socket) {
// unref socket to prevent it from keeping the process running
this._sender._client.unref();
Copy link
Member

Choose a reason for hiding this comment

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

👍

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 23, 2020
@dyladan dyladan merged commit 801df25 into open-telemetry:master Jan 24, 2020
@dyladan dyladan deleted the unref-jaeger branch January 24, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@OlivierAlbertini OlivierAlbertini OlivierAlbertini approved these changes

@bg451 bg451 Awaiting requested review from bg451

@markwolff markwolff Awaiting requested review from markwolff

@obecny obecny Awaiting requested review from obecny

@rochdev rochdev Awaiting requested review from rochdev

@vmarchaud vmarchaud Awaiting requested review from vmarchaud

Assignees
No one assigned
Labels
bug Something isn't working Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Jaeger exporter keeps program open indefinitely
4 participants
@dyladan @codecov-io @OlivierAlbertini @mayurkale22