Report error ingestion through OpenTelemetry (3/4) - #5816
Merged
Conversation
johnsimons
force-pushed
the
john/ingestion_otel
branch
from
August 24, 2026 02:37
c48d6af to
b11ad24
Compare
rbev
approved these changes
Aug 24, 2026
|
|
||
| public bool PrintMetrics => SettingsReader.Read<bool>(SettingsRootNamespace, "PrintMetrics"); | ||
|
|
||
| public string OtlpEndpointUrl { get; set; } = SettingsReader.Read<string>(SettingsRootNamespace, nameof(OtlpEndpointUrl)); |
Contributor
There was a problem hiding this comment.
Should this env var fall back to the otel specification naming: OTEL_EXPORTER_OTLP_ENDPOINT?
You might be able to remove all the configuration as this is a standard that I believe is automatically picked up by the call to WithMetrics.
Member
Author
There was a problem hiding this comment.
I just followed what is already in Audit
The audit instance has had OpenTelemetry ingestion metrics for a while: batch and per message durations as histograms, a failure counter, and a gauge for consecutive batch failures, all exportable over OTLP and graphable. The error instance had counters that a hosted service printed to the log every five seconds when PrintMetrics was on, which cannot be graphed and cannot be alerted on. The error instance now reports the same shape under sc.error.ingestion.* on the meter Particular.ServiceControl, with OtlpEndpointUrl wired up the same way it is on the audit instance. Instrument names are what dashboards are built on, so both instances now have a test that pins theirs, and the audit names are unchanged. Where the two genuinely differ: - The error side keeps the bulk insert timing it already had, as sc.error.ingestion.storage_duration_seconds. It is the part of a batch that is neither announcing nor forwarding, and the audit ingestor has no equivalent. - message.category splits retry-confirmation from failed-message, which is the same split ErrorIngestor makes when it divides a batch, and the two cost very different amounts of work. The scopes behind all of this move into ServiceControl.Infrastructure: one batch, one message, one failure, one plain duration, and the histogram buckets they share so both instances stay comparable on the same dashboard. What stays per instance is what actually differs, which is instrument names, descriptions and the tags. Hosting audit ingestion in the primary will pick these up rather than copy them. The legacy counters leave the ingestion path, and with them the last thing that called Metrics.GetCounter or GetMeter anywhere. The reporter and PrintMetrics stay registered and now have nothing to report, so removing that plumbing, and the setting with it, is a decision on its own rather than a side effect of this. The consecutive batch failure gauge is updated with Interlocked and read with Volatile.Read. It was a plain field, which was safe while one writer owned the loop and loses increments now that several batches can fail at once, on the one number an alert would be built on.
johnsimons
force-pushed
the
john/ingestion_otel
branch
from
August 24, 2026 09:39
b11ad24 to
7bf4fcf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 3 of 4 in a series that reworks how both instances ingest messages. It comes out of the parallel audit writers spike in #5318. Master has moved a long way past that spike's base, so this ports the ideas rather than the files.
Each branch is a single commit and stacks on the one before it, so review them in order. 1/4 is based on master. The series is independent of #NNNN ("Order the failed message status by the times the events happened"), which is a standalone bug fix going out alongside it.
Why
The audit instance has had OpenTelemetry ingestion metrics for a while: batch and per message durations as histograms, a failure counter, and a gauge for consecutive batch failures, all exportable over OTLP and graphable. The error instance had counters that a hosted service printed to the log every five seconds when
PrintMetricswas on, which cannot be graphed and cannot be alerted on. Now that writer count and batch size are tunable, the numbers that tell you how to set them have to be visible.What this does
The error instance reports the same shape as audit, under
sc.error.ingestion.*on the meterParticular.ServiceControl, withOtlpEndpointUrlwired up the same way it is on the audit instance.Instrument names are what dashboards are built on, so both instances now have a test that pins theirs. The audit names are unchanged.
Where the two genuinely differ:
sc.error.ingestion.storage_duration_seconds. It is the part of a batch that is neither announcing nor forwarding, and the audit ingestor has no equivalent.message.categorysplits retry confirmations from failed messages, which is the same splitErrorIngestormakes when it divides a batch. The two cost very different amounts of work, so averaging them together hides both.The scopes behind all of this move into
ServiceControl.Infrastructure: one batch, one message, one failure, one plain duration, and the histogram buckets they share, so both instances stay comparable on the same dashboard. What stays per instance is what actually differs, which is instrument names, descriptions and tags. Hosting audit ingestion in the primary will pick these up rather than copy them.The failure gauge is now atomic
consecutive_batch_failures_totalwas backed by a plain field, which was safe while one writer owned the loop. With parallel writers it loses increments on the one number an alert would be built on, so it is updated withInterlockedand read withVolatile.Read. Both instances have a test that fails a thousand batches concurrently and checks the gauge; it reads short without the fix.The legacy counters
They leave the ingestion path, and with them the last thing that called
Metrics.GetCounterorGetMeteranywhere. The reporter andPrintMetricsstay registered and now have nothing to report. Removing that plumbing, and the setting with it, is a decision on its own rather than a side effect of this PR.