-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
feat: Collect duplicate log line metrics #13084
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 |
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.
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)
pkg/ingester/stream.go
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: I think it makes sense to unify both into a single config.
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.
i think i would leave these separate, I think generally just the metric is useful?
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.
LGTM!
I think my only nit would be to add a comment to each of the Append implementations which explains what the bool does
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.md
guide (required)feat
PRs 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.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR