-
Notifications
You must be signed in to change notification settings - Fork 821
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
Jaeger no flush interval #609
Jaeger no flush interval #609
Conversation
Codecov Report
@@ Coverage Diff @@
## master #609 +/- ##
=========================================
- Coverage 92.13% 90.44% -1.7%
=========================================
Files 182 180 -2
Lines 9041 9135 +94
Branches 792 814 +22
=========================================
- Hits 8330 8262 -68
- Misses 711 873 +162
|
Brought over from issue thread |
this._logger.debug('successful append for : %s', thriftSpan.length); | ||
|
||
// Flush all spans on each export. No-op if span buffer is empty | ||
await this._flush(); |
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.
Perhaps flush
should happen whenever thriftSpan.length > 0
and move above debug
statement in same if
loop. WDYT?
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.
First thing the sender does is check length and return if zero and this isn't a critical path or tight loop. Can change if you feel strongly though
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.
Ok. I was little annoyed by the above debug log, even though nothing was exported it just printed successful append for : 0
every few secs.
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.
Oh i understand, yea that can be fixed
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.
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.
overall lgtm
I also added e7d0695 which makes the batch span processor periodic timer not export if there is nothing to export |
Which problem is this PR solving?
Short description of the changes