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

chore(aggregated_metrics): Add metrics to aggreated metrics #14986

Merged
merged 10 commits into from
Nov 22, 2024

Conversation

shantanualsi
Copy link
Contributor

@shantanualsi shantanualsi commented Nov 18, 2024

What this PR does / why we need it:
Adds metrics to aggregated metrics to better quantify the subsystem and also create a dashboard.

Which issue(s) this PR fixes:

Special notes for your reviewer:

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

@shantanualsi shantanualsi marked this pull request as ready for review November 18, 2024 12:50
@shantanualsi shantanualsi requested a review from a team as a code owner November 18, 2024 12:50
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

thanks for adding this! just a few small things

@@ -142,6 +147,7 @@ func NewPush(
entries: entries{
entries: make([]entry, 0),
},
metrics: NewMetrics(registrer, "pattern_ingester"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the namespace be loki here? we already have the subsystem as pattern_ingester, so I think this would make the metric have pattern_ingester in it twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, missed this one. Fixed.

Comment on lines 231 to 233
p.metrics.streamsPerPush.WithLabelValues(p.tenantID).Observe(float64(len(streams)))
p.metrics.entriesPerPush.WithLabelValues(p.tenantID).Observe(float64(len(entries)))
p.metrics.servicesTracked.WithLabelValues(p.tenantID).Set(float64(len(entriesByStream)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference between number of streams and the length of the entries by stream map? won't those always be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, was supposed to count services tracked.

@@ -287,6 +297,31 @@ func (p *Push) run(pushPeriod time.Duration) {
}
}

func (p *Push) sendPayload(ctx context.Context, payload []byte) (int, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find any references to this new function, is it being used?

if err != nil {
errorType := "unknown"
if status == 429 {
errorType = "rate_limited"
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we move these to constants so their easier to find, and test against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, moved this to constants

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

lgtm, thanks Shantanu!

@shantanualsi shantanualsi merged commit d3d31f1 into main Nov 22, 2024
57 checks passed
@shantanualsi shantanualsi deleted the shantanu/aggregated-metrics-metrics branch November 22, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants