Performance optimization for live debugging feature #2294
Conversation
ravishankar15
requested review from
clayton-cornell and
a team
as code owners
ravishankar15
force-pushed
the
live-debug-performance
branch
from
5ef00a5 to
574403a
Compare
| @@ -164,7 +165,7 @@ func TestJSONLabelsStage(t *testing.T) { | |||
| "[IN]: timestamp: 2020-11-15T02:08:41-07:00, entry: {\"log\":\"log message\\n\",\"stream\":\"stderr\",\"time\":\"2019-04-30T02:12:41.8443515Z\",\"extra\":\"{\\\"user\\\":\\\"smith\\\"}\"}, labels: {filename=\"/var/log/pods/agent/agent/1.log\", foo=\"bar\"}", | |||
| "[OUT]: timestamp: 2019-04-30T02:12:41.8443515Z, entry: {\"log\":\"log message\\n\",\"stream\":\"stderr\",\"time\":\"2019-04-30T02:12:41.8443515Z\",\"extra\":\"{\\\"user\\\":\\\"smith\\\"}\"}, labels: {filename=\"/var/log/pods/agent/agent/1.log\", foo=\"bar\", stream=\"stderr\", ts=\"2019-04-30T02:12:41.8443515Z\", user=\"smith\"}", | |||
| } | |||
| require.Equal(t, expectedLiveDebuggingLog, liveDebuggingLog.Get()) | |||
| assert.ElementsMatch(t, expectedLiveDebuggingLog, liveDebuggingLog.Get()) | |||
There was a problem hiding this comment.
this test is not flaky on main, why would it sometimes not be ordered with your changes?
There was a problem hiding this comment.
It was failing in my earlier commit https://github.com/grafana/alloy/actions/runs/12394965463/job/34599585193
There was a problem hiding this comment.
yes but we need to figure out why that's the case. The order is important, if you receive the "out" before the "in" then it's confusing in the UI
There was a problem hiding this comment.
Hey @wildum I missed adding the channel size in the tests have added the same. But I am not sure if that is the reason for failure. I checked the code and logic seems to be good I don't see any race conditions. Like you mentioned the IN and OUT are published in order and should be received in order. I tried to run this in local a couple of times frequently with the channel size fix it seems stable. Can you help me see if I am missing any race condition on this test failure ?
There was a problem hiding this comment.
that's weird, I can't also reproduce it anymore, even without the buffer size 🤔
But in the meantime I ran some tests and even under heavy load a small buffer size was not dropping data. The front-end will have trouble to keep up before the buffer is threatened. I think that it's not worth it to make it a parameter in the end. We can keep the flushing at regular interval though but for the buffer I would just remove the TODO comment. Sorry for the trouble
There was a problem hiding this comment.
ohh, You mean we should remove the config buffer_stream_size ? and have only the flush interval changes ? or just remove the config changes for the tests made in this commit
There was a problem hiding this comment.
I think that we should remove the config buffer_stream_size because it doesn't seem necessary in the end
ravishankar15
force-pushed
the
live-debug-performance
branch
2 times, most recently
from
d6f8972 to
574403a
Compare
PR Description
Which issue(s) this PR fixes
Fixes #974
Notes to the Reviewer
PR Checklist