-
Notifications
You must be signed in to change notification settings - Fork 478
remove labelstore from prometheus interceptor #4890
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| return &Fanout{ | ||
| children: children, | ||
| componentID: componentID, |
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.
Drive-by: This wasn't referenced anywhere
50b5eca to
a27d37a
Compare
a27d37a to
83fb65a
Compare
| "github.com/grafana/alloy/syntax" | ||
| ) | ||
|
|
||
| func TestRelabelThroughAppend(t *testing.T) { |
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 is covered through the relabel pipeline test
| if len(res.Timeseries) == 1 { | ||
| results = append(results, res.Timeseries[0]) | ||
| } else if len(res.Timeseries) == 2 { | ||
| results = res.Timeseries | ||
| } | ||
| // When we have two results make sure they match what we expect | ||
| if len(results) == 2 { | ||
| require.Equal(t, expect, results) | ||
| } |
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.
Drive-by: This test failed so I thought I broke it but it's just flaky since it's possible one metric is remote written without the other.
go test ./internal/component/prometheus/remotewrite/... -run '^Test$' -count 20
used to break it locally
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.
Don't we want to have a for loop to ensure we eventually write both of the time series?
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.
Or we can change the remote write config to make sure both metrics are remote written at the same time? I'm not sure if that's possible though. Maybe we can set max_samples_per_send to 2 and batch_send_deadline to an hour? 😆
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.
Ugh yeah you're right this still isn't right. I'll fix it properly this time.
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 went with the max samples + batch send that matches the test timeout, thanks for the idea!
ptodev
left a comment
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'm not super familiar with the label store, but on a high level this change makes sense. I'm just not sure why the Fanout is a better place for a cache than Interceptor? In theory should the Interceptor be better, since it's the entry point to a component?
| if len(res.Timeseries) == 1 { | ||
| results = append(results, res.Timeseries[0]) | ||
| } else if len(res.Timeseries) == 2 { | ||
| results = res.Timeseries | ||
| } | ||
| // When we have two results make sure they match what we expect | ||
| if len(results) == 2 { | ||
| require.Equal(t, expect, results) | ||
| } |
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.
Don't we want to have a for loop to ensure we eventually write both of the time series?
| if len(res.Timeseries) == 1 { | ||
| results = append(results, res.Timeseries[0]) | ||
| } else if len(res.Timeseries) == 2 { | ||
| results = res.Timeseries | ||
| } | ||
| // When we have two results make sure they match what we expect | ||
| if len(results) == 2 { | ||
| require.Equal(t, expect, results) | ||
| } |
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.
Or we can change the remote write config to make sure both metrics are remote written at the same time? I'm not sure if that's possible though. Maybe we can set max_samples_per_send to 2 and batch_send_deadline to an hour? 😆
|
|
||
| - Add `meta_cache_address` to `beyla.ebpf` component. (@skl) | ||
|
|
||
| - Remove labelstore interactions from the prometheus interceptor simplifying prometheus pipelines. (@kgeckhart) |
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.
Does this change really need a changelog entry? It is more like an internal refinment that doesn't impact users.
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.
🤷 it's an optimization on a highly used pipeline
| } | ||
|
|
||
| interceptor := c.newInterceptor(ls) | ||
| interceptor := NewInterceptor(livedebugging.ComponentID(o.ID), c.debugDataPublisher, alloyAppendable) |
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.
It's interesting that components such as scrape and receive_http also have an interceptor 🤔 It's not just for downstream components that have metrics streamed to them by upstream components.
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'll comment on this in regards to why fanout is makes more sense for the labelstore interaction vs the interceptor. receive_http does not actually use the interceptor because it has no further logic to inject it only uses fanout.
It's a good question that I didn't expand upon in the PR. I added a comment to Fanout to indicate it's role with global series refs. |
PR Description
Removes unnecessary usage of labelstore from the prometheus.interceptor used in prometheus pipelines. Beyond simplifying the code there's a performance gain from the reduction in duplicate staleness tracking through the labelstore,
The results were generated through the "headless" prometheus pipeline tests I added to track end-to-end scenarios related to WAL functionality that will be important as the labelstore continues to evolve.
Notes to the Reviewer
There's no need to have the interceptor also handle labelstore interactions as everything that uses the interceptor is also using
prometheus.Fanoutwhich is the most ideal entrypoint for labelstore interactions.PR Checklist