Sync/Async producers, retries and batch dispatch #1
Conversation
ghostec
commented
May 9, 2019
•
ghostec
commented
- use rollup to distribuite lib/es with babel transpillation
- reorganize project to split producers into 2
- Async producer with SendEvents (batch) method
- Async producer Retry logic
- Report DroppedEvents
ghostec
force-pushed
the
feature/async-batches-retries
branch
10 times, most recently
from
d355f2d
to
5972ae5
Compare
ghostec
changed the title
ghostec
force-pushed
the
feature/async-batches-retries
branch
3 times, most recently
from
0c21a3a
to
c8e53c0
Compare
src/client.js
Outdated
| // the proposal was already accepted | ||
| // https://github.com/grpc/proposal/blob/master/L5-NODEJS-CLIENT-INTERCEPTORS.md | ||
| // but the PR is still ongoing | ||
| // https://github.com/grpc/grpc-node/pull/59 |
There was a problem hiding this comment.
It looks like the PR was already merged.
There was a problem hiding this comment.
I checked this yesterday but I wasn't sold on the idea that we gain a lot by changing how it already is done. What are your points about using an interceptor vs what we have right now?
There was a problem hiding this comment.
I don't have a strong opinion actually. Using the interceptor is a bit cleaner but my main issue here is that:
- The comment is no longer valid since the PR is merged.
- It is in the wrong place. We would use the interceptor in
this.grpcClientand not when building the event, right?
If we choose to move on without the interceptor I'd just remove this comment to avoid future confusion.
There was a problem hiding this comment.
alright! removing the comment
src/lib/metricsPrometheus.js
Outdated
| @@ -34,14 +34,20 @@ const clientRequestsFailureCounter = new client.Counter({ | |||
| labelNames: ['clientHost', 'route', 'topic', 'reason'], | |||
There was a problem hiding this comment.
I don't think we need the clientHost as tag.
There was a problem hiding this comment.
this is the same as go client
src/lib/metricsReporter.js
Outdated
|
|
||
| class MetricsReporter { | ||
| constructor(config) { | ||
| this.hostname = os.hostname() |
There was a problem hiding this comment.
We're no longer using hostname tag on our metrics. Remove this, please.
There was a problem hiding this comment.
alright! will remove from go client as well
There was a problem hiding this comment.
Great!
This solves the comment about clientHost as well, right?
src/producer/async.js
Outdated
| this.logger = logger.child({ address: this.address }) | ||
| this.metrics = new MetricsReporter(this.config) | ||
| this.method = '/eventsgateway.GRPCForwarder/SendEvents' | ||
| this.lingerIntervalMs = this.config.producer.lingerIntervalMs === undefined ? |
There was a problem hiding this comment.
I'm not a fan of these long ternary lines. Maybe have a method like this (with better naming, please)
configure(field, default) {
const val = this.config.producer[field]
this[field] = val === undefined ? default : val
}
and inside the constructor use it:
this.configure('lingerIntervalMs', 500)
src/producer/sync.js
Outdated
| }) | ||
| } | ||
|
|
||
| async gracefulStop () { |
There was a problem hiding this comment.
It bugs me that we have some duplicate code from async producer. Can't we share it?
There was a problem hiding this comment.
added producer/waitGroup
ghostec
force-pushed
the
feature/async-batches-retries
branch
from
c8e53c0
to
d6b99f1
Compare
