Change OC receiver behavior according to host ingestion status #192
Conversation
Make OC receiver respond to host ingestion status.
pjanotti
requested review from
bogdandrutu,
flands,
songy23 and
tigrannajaryan
as code owners
| }) | ||
| } | ||
|
|
||
| // TODO: Create proper observability some all metric receivers generate |
There was a problem hiding this comment.
Opened #193 to track this and just noticed a typo here :)
| for _, md := range mds { | ||
| // TODO: add observability for errors here. | ||
| // TODO: consider queued-retry for metrics too. | ||
| ocr.nextConsumer.ConsumeMetricsData(ctx, *md) |
There was a problem hiding this comment.
The usage of the bundler here doesn't seem correct, follow up #194
Codecov Report
@@ Coverage Diff @@
## master #192 +/- ##
==========================================
+ Coverage 72.74% 72.93% +0.18%
==========================================
Files 104 104
Lines 6094 6118 +24
==========================================
+ Hits 4433 4462 +29
+ Misses 1435 1429 -6
- Partials 226 227 +1
Continue to review full report at Codecov.
|
| } | ||
| for _, opt := range opts { | ||
| opt(ocr) | ||
| } | ||
|
|
||
| // Setup and startup worker pool | ||
| workers := make([]*receiverWorker, 0, ocr.numWorkers) |
There was a problem hiding this comment.
They are not really needed: see #169
|
|
||
| type receiverWorker struct { |
There was a problem hiding this comment.
Looks like you removed workers. Was it supposed to be in this PR?
There was a problem hiding this comment.
It was intentional since I was already touching this code and this reduces code. Notice that concurrency them becomes more "controlled" via the pipeline using and configuring processors like batcher and queued-retry.
| errChan <- ocr.httpServer().Serve(httpL) | ||
| }() | ||
| errChan <- m.Serve() | ||
| ocr.host.ReportFatalError(ocr.serverGRPC.Serve(grpcL)) |
There was a problem hiding this comment.
Isn't it needed to check if Serve() returns nil?
There was a problem hiding this comment.
Good question.
In practice these are all blocking calls that will only return when the server is closed (due to errors or explicit stop/close). Prior to a shutdown we want to be notified if they terminate (even if returning nil) since it means that the receiver in question is not running anymore - that said it seems a server implementation error if they return nil without corresponding stop/close call (in such case the log message won't be helpful at all).
In a shutdown triggered via ReportFatalError, ie.: "serve" call returning, only the first one is reported in the logs. For a shutdown via SIGTERM no errors reported via ReportFatalError are logged. Notice that it is implementation dependent if the "Serve" will return nil or an error in case the corresponding stop/close is called: e.g.: http always returns error while grpc can return nil if stop/close is called.
| ocr.host.ReportFatalError(ocr.serverGRPC.Serve(grpcL)) | ||
| }() | ||
| go func() { | ||
| ocr.host.ReportFatalError(ocr.httpServer().Serve(httpL)) |
| ocr.host.ReportFatalError(ocr.httpServer().Serve(httpL)) | ||
| }() | ||
| go func() { | ||
| ocr.host.ReportFatalError(m.Serve()) |
|
We are not going to move ahead with this (further discussion and design related to #76). Closing this PR. |
…open-telemetry#1660) * Remove `WithRecord()` option from SpanConfig options This brings the trace API into conformance with the specification. * Add entry to CHANGELOG Fixes open-telemetry#192 * Updated CHANGELOG with PR# * Cleaned up CHANGELOG notes * fixup! Merge remote-tracking branch 'upstream/main' into remove-with-record * Use new spanContext API to set traceflags, tracestate Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
* Add Traces support to SA Receiver * only check SFx span nil tags once
Make OC receiver respond to host ingestion status. This is part of #76
This closes #171, and closes #169.