Skip to content
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

app/vmagent: added global aggregator #6268

Merged
merged 2 commits into from
May 17, 2024
Merged

app/vmagent: added global aggregator #6268

merged 2 commits into from
May 17, 2024

Conversation

AndrewChubatiuk
Copy link
Contributor

Describe Your Changes

Added global stream aggregation for VMAgent

Checklist

The following checks are mandatory:

@AndrewChubatiuk AndrewChubatiuk force-pushed the aggregators-global branch 4 times, most recently from 83e72d9 to 26e09dc Compare May 14, 2024 12:43
Copy link
Collaborator

@hagen1778 hagen1778 left a comment

Choose a reason for hiding this comment

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

We should outline in docs and flag description the main purpose of the global aggregation config: to fan-out aggregated data to multiple URLs.
Have you tried comparing resource usage of aggregating data globally and per-url before sending to 2 destinations?

@@ -491,13 +479,28 @@ func tryPush(at *auth.Token, wr *prompbmarshal.WriteRequest, forceDropSamplesOnF
}
sortLabelsIfNeeded(tssBlock)
tssBlock = limitSeriesCardinality(tssBlock)
sas := sasGlobal.Load()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we retrieve sas before for len(tss) > 0 { on L:448?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it

app/vmagent/remotewrite/streamaggr.go Show resolved Hide resolved
@AndrewChubatiuk
Copy link
Contributor Author

We should outline in docs and flag description the main purpose of the global aggregation config: to fan-out aggregated data to multiple URLs. Have you tried comparing resource usage of aggregating data globally and per-url before sending to 2 destinations?

yes, tested it and there was a significant difference in resource usage

@AndrewChubatiuk AndrewChubatiuk force-pushed the aggregators-global branch 6 times, most recently from 39caea2 to 0dfb022 Compare May 15, 2024 13:50
* clarify the deprecation note in Changelog
* explain the purpose of the stream aggregation change in Changelog
* remove mentions of `global` aggregation from docs.
Use it as usual and per-url aggregation in wording, so it comes as default aggregation option (wide case)
and then user can apply it per-url (narrow case)
* add #Routing section to stream aggregation docs
* update vmagent's list of flags

Signed-off-by: hagen1778 <roman@victoriametrics.com>
@hagen1778
Copy link
Collaborator

@AndrewChubatiuk please see changes at 8fac4a6
If you're happy with updates, I'm going to merge the PR.

@AndrewChubatiuk
Copy link
Contributor Author

@AndrewChubatiuk please see changes at 8fac4a6 If you're happy with updates, I'm going to merge the PR.

yes, let's merge it

@hagen1778 hagen1778 merged commit f153f54 into master May 17, 2024
8 checks passed
@hagen1778 hagen1778 deleted the aggregators-global branch May 17, 2024 12:00
hagen1778 pushed a commit that referenced this pull request May 17, 2024
Add global stream aggregation for VMAgent

#5467
(cherry picked from commit f153f54)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants