fix: Fix push stats calculation #19319
Conversation
|
|
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| shouldDiscoverServiceName := len(discoverServiceName) > 0 && !stats.IsInternalStream | ||
| shouldDiscoverServiceName := len(discoverServiceName) > 0 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
I'm assuming the extra spaces are important for the assertion on stream label size? I think that's worth adding to the comment
What this PR does / why we need it:
Current calculation of push
StatsinParseLokiRequestis based on data that might not be available later inPushParserWrapper. This leads to mismatched calculations in AdaptiveLogs wrapper and it returns negative stats as a result.pushStats.StreamLabelsSizeis 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 (inPushParserWrapper).util.StructuredMetadataSizewas not used in the wrapper, that led to mismatched numbers for structured metadata.isInternalStreamis set to true even if there is only one internal stream in the whole request. Might affect other streams in the same request.Extracted
CalculateStreamsStatsso 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
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.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR