feat: Collect duplicate log line metrics - #13084
Conversation
|
I think rather than incrementing the metric in 2 places, is it possible instead to increment it inside the unordered head block? this is the "source of truth" if you will for if we store an exact dupe or not. Also then we can avoid returning an error from that function which I think is changing some existing behaviors in Loki |
github-actions
Bot
added
the
type/docs
DylanGuedes
left a comment
DylanGuedes
left a comment
There was a problem hiding this comment.
sounds good, just a few small suggestions and one change necessary (the one regarding not logging anything on the manager if the new arg is false)
| if dup { | ||
| if s.configs != nil { | ||
| if s.configs.LogDuplicateMetrics(s.tenant) { | ||
| s.metrics.duplicateLogBytesTotal.WithLabelValues(s.tenant).Add(float64(len(entries[i].Line))) |
There was a problem hiding this comment.
this PR adds two CLI configs, one for the metric and one for the logs. WDYT of making both working behind a single one? Asking because I can't think of a scenario were I'd want only one of them. But having both means another CLI flag to Loki.
There was a problem hiding this comment.
@slim-bean Argues that we need two, which is why we added it
| # Log metrics for duplicate lines received. | ||
| # CLI flag: -operation-config.log-duplicate-metrics | ||
| [log_duplicate_metrics: <boolean> | default = false] | ||
|
|
||
| # Log stream info for duplicate lines received | ||
| # CLI flag: -operation-config.log-duplicate-stream-info | ||
| [log_duplicate_stream_info: <boolean> | default = false] |
There was a problem hiding this comment.
ditto: I think it makes sense to unify both into a single config.
There was a problem hiding this comment.
i think i would leave these separate, I think generally just the metric is useful?
slim-bean
left a comment
slim-bean
left a comment
There was a problem hiding this comment.
LGTM!
I think my only nit would be to add a comment to each of the Append implementations which explains what the bool does
3 participants
What this PR does / why we need it:
This PR is for adding support to collect metrics and information about duplicate logs that are ingested. Previously, duplicate logs are just discarded, with no way to actually see the fact that duplicates were received, nor the quantity of the data that was discarded.
This PR adds two runtime configuration options for a tenant:
log_duplicate_metrics: To output an ingester metric namedduplicate_log_bytes_total, by tenant id, which counts how many bytes were discarded due to log line(s) being duplicates.log_duplicate_stream_info: To output details about duplicate logs to insight logs.Which issue(s) this PR fixes:
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR