-
Notifications
You must be signed in to change notification settings - Fork 795
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.
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
chore: adding force flush to span processors #802
Changes from 1 commit
b7fa9e8
dcfdd06
8e3ae1b
076d9ad
0aac34a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,16 +27,29 @@ import { Span } from '../Span'; | |
*/ | ||
export class SimpleSpanProcessor implements SpanProcessor { | ||
constructor(private readonly _exporter: SpanExporter) {} | ||
private _isShutdown = false; | ||
|
||
forceFlush(): void { | ||
// do nothing as all spans are being exported without waiting | ||
} | ||
|
||
// does nothing. | ||
onStart(span: Span): void {} | ||
|
||
onEnd(span: Span): void { | ||
if (this._isShutdown) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually agree. if the span processor shuts down its exporter, then it should be safe for it to continue sending spans even after shutdown and the exporter will just no-op There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but this is what spec says for span processors There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one additional |
||
return; | ||
} | ||
if (span.context().traceFlags !== TraceFlags.SAMPLED) return; | ||
this._exporter.export([span.toReadableSpan()], () => {}); | ||
} | ||
|
||
shutdown(): void { | ||
if (this._isShutdown) { | ||
return; | ||
} | ||
this._isShutdown = true; | ||
|
||
this._exporter.shutdown(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove
_forceFlushOnShutdown
from Jaeger exporter also.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In zipkin this functionality was in fact "dead" but for Jaeger it does something - there is a timeout which waits for the Jaeger to close the sender. Are you ok to make it in separate PR then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure 👍