Skip to content

fix: Fix push stats calculation - #19319

Merged
spiridonov merged 6 commits into
mainfrom
spiridonov-fix-push-stats
Sep 29, 2025
Merged

fix: Fix push stats calculation#19319
spiridonov merged 6 commits into
mainfrom
spiridonov-fix-push-stats

Conversation

@spiridonov

@spiridonov spiridonov commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
Current calculation of push Stats in ParseLokiRequest is based on data that might not be available later in PushParserWrapper. This leads to mismatched calculations in AdaptiveLogs wrapper and it returns negative stats as a result.

  • pushStats.StreamLabelsSize is a size of labels BEFORE they are sanitized (only relevant to protobuf payloads). This is not a big deal, unless a new label is added for service name. Then labels are modified inside the original stream object and it is impossible to recover the original size (in PushParserWrapper).
  • util.StructuredMetadataSize was not used in the wrapper, that led to mismatched numbers for structured metadata.
  • isInternalStream is set to true even if there is only one internal stream in the whole request. Might affect other streams in the same request.

Extracted CalculateStreamsStats so that it can be reused in a wrapper. It takes a push request and calculates all stream stats for it. Adaptive Logs now can drop some streams/entries from a request and just call this method in order to get new stats, instead of subtracting values for dropped items.

Minor: also added some comments, and fixed few typos.

Special notes for your reviewer:
There will be a follow up PR for GEL.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@spiridonov
spiridonov requested a review from a team as a code owner September 25, 2025 17:27
@CLAassistant

CLAassistant commented Sep 25, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@spiridonov
spiridonov marked this pull request as draft September 26, 2025 13:02
@spiridonov
spiridonov marked this pull request as ready for review September 26, 2025 17:23
@spiridonov
spiridonov enabled auto-merge (squash) September 26, 2025 17:23
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Sep 29, 2025

@trevorwhitney trevorwhitney left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple small things.

Also, in response to setting isInternalStream to true even if there is only one internal stream in the whole request, I think your refactor is better as it keeps more state local to the function, but in practice a push req should never have a mix of internal and tenant provided streams. The internal streams come from us, and never contain tenant provided streams, so if a req does have both it means the tenant is doing something nefarious.

Comment thread pkg/loghttp/push/otlp.go
}

shouldDiscoverServiceName := len(discoverServiceName) > 0 && !stats.IsInternalStream
shouldDiscoverServiceName := len(discoverServiceName) > 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove the extra check? we shouldn't be running service name detection on payloads with internal streams

&logproto.PushRequest{
Streams: []logproto.Stream{
{
Labels: `{ "foo"="bar2", "environment"="prod" }`, // extra spaces are important

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the extra spaces are important for the assertion on stream label size? I think that's worth adding to the comment

@spiridonov
spiridonov merged commit a6d4935 into main Sep 29, 2025
65 checks passed
@spiridonov
spiridonov deleted the spiridonov-fix-push-stats branch September 29, 2025 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 participants