Send target and metadata cache in context (again) #10636

gouthamve · 2022-04-27T18:03:47Z

The previous attempt was rolled back in #10590 due to memory issues.

sl.parentCtx and sl.ctx both had a copy of the cache and target info
in the previous attempt and it was hard to pin-point where the context
was being retained causing the memory increase.

I've experimented a bunch in #10627 to figure out that this approach doesn't
cause memory increase. Beyond that, just using this info in any other context
is causing a memory increase. I'm hoping just this won't cause issues but doing the following on top:

diff --git a/scrape/scrape.go b/scrape/scrape.go
index 76739b9cb..8efa431f0 100644
--- a/scrape/scrape.go
+++ b/scrape/scrape.go
@@ -1288,7 +1288,7 @@ func (sl *scrapeLoop) scrapeAndReport(last, appendTime time.Time, errc chan<- er
        }

        var contentType string
-       scrapeCtx, cancel := context.WithTimeout(sl.parentCtx, sl.timeout)
+       scrapeCtx, cancel := context.WithTimeout(sl.appCtx, sl.timeout)
        contentType, scrapeErr = sl.scraper.scrape(scrapeCtx, buf)
        cancel()

still causes a slight memory increase. So at this point, I'm not sure how to debug it.

The change fixed a bunch of long-standing in the OTel Collector that the
community was waiting on and release is blocked on a few downstream distrubutions
of OTel Collector waiting on a fix. I propose to merge this change in while
I investigate what is happening.

gouthamve · 2022-04-27T18:03:59Z

/prombench main

prombot · 2022-04-27T18:04:00Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-10636 and main

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

roidelapluie · 2022-04-27T18:15:23Z

Can you gate this behind a manager option so Prometheus is not affected?
Can we design this in another way that does not involve context?

gouthamve · 2022-04-28T12:36:47Z

Can you gate this behind a manager option so Prometheus is not affected?

Yes. I can do this.

Can we design this in another way that does not involve context?

I've looked into this before and couldn't find a better idea. The problem is OTel needs to look up metadata for each sample added in Append, and the only thing we provide is scrape.NewManager which only takes an Appendable interface. I'll take another look today

gouthamve · 2022-04-28T12:42:21Z

/prombench cancel

prombot · 2022-04-28T12:42:22Z

Benchmark cancel is in progress.

gouthamve · 2022-05-02T18:43:41Z

@roidelapluie Gated it behind a manager option. Could you take a look?

The previous attempt was rolled back in #10590 due to memory issues. `sl.parentCtx` and `sl.ctx` both had a copy of the cache and target info in the previous attempt and it was hard to pin-point where the context was being retained causing the memory increase. I've experimented a bunch in #10627 to figure out that this approach doesn't cause memory increase. Beyond that, just using this info in _any_ other context is causing a memory increase. The change fixed a bunch of long-standing in the OTel Collector that the community was waiting on and release is blocked on a few downstream distrubutions of OTel Collector waiting on a fix. I propose to merge this change in while I investigate what is happening. Signed-off-by: Goutham Veeramachaneni <[email protected]>

roidelapluie

LGTM after lint fixes

Signed-off-by: Goutham Veeramachaneni <[email protected]>

gouthamve · 2022-05-03T01:08:45Z

Fixed.

Nexucis · 2022-05-03T09:00:19Z

wouldn't be nice to have it as a fix for the v2.35.x series ?

roidelapluie · 2022-05-03T09:07:33Z

wouldn't be nice to have it as a fix for the v2.35.x series ?

There is no end user impact here, I would not release prometheus for it. as a stop gap, otel can point to the commit

roidelapluie

LGTM

Prometheus started doing proper go.mod publishing and started off with v0.35, but this means the previous version was "winning" as it was v1.8.2 so I had to update the version everywhere. This brings in prometheus/prometheus#10636 which fixes open-telemetry#9278 Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Update Prometheus to not have memory leak. Prometheus started doing proper go.mod publishing and started off with v0.35, but this means the previous version was "winning" as it was v1.8.2 so I had to update the version everywhere. This brings in prometheus/prometheus#10636 which fixes #9278 Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Update Prometheus to not have memory leak. Prometheus started doing proper go.mod publishing and started off with v0.35, but this means the previous version was "winning" as it was v1.8.2 so I had to update the version everywhere. This brings in prometheus/prometheus#10636 which fixes open-telemetry#9278 Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Send target and metadata cache in context (again) The previous attempt was rolled back in prometheus#10590 due to memory issues. `sl.parentCtx` and `sl.ctx` both had a copy of the cache and target info in the previous attempt and it was hard to pin-point where the context was being retained causing the memory increase. I've experimented a bunch in prometheus#10627 to figure out that this approach doesn't cause memory increase. Beyond that, just using this info in _any_ other context is causing a memory increase. The change fixed a bunch of long-standing in the OTel Collector that the community was waiting on and release is blocked on a few downstream distrubutions of OTel Collector waiting on a fix. I propose to merge this change in while I investigate what is happening. Signed-off-by: Goutham Veeramachaneni <[email protected]> * Gate the change behind a manager option Signed-off-by: Goutham Veeramachaneni <[email protected]>

prombot · 2022-08-18T18:53:33Z

@khanhntd is not a org member nor a collaborator and cannot execute benchmarks.

prombot added the prombench label Apr 27, 2022

gouthamve requested a review from roidelapluie April 27, 2022 18:05

gouthamve mentioned this pull request Apr 28, 2022

WIP: Prombench Figure out where the leak is #10627

Closed

gouthamve force-pushed the otel-context-metadata branch from d276fe8 to d1176d0 Compare May 2, 2022 21:13

roidelapluie reviewed May 2, 2022

View reviewed changes

Gate the change behind a manager option

5808889

Signed-off-by: Goutham Veeramachaneni <[email protected]>

gouthamve force-pushed the otel-context-metadata branch from d1176d0 to 5808889 Compare May 3, 2022 00:56

roidelapluie approved these changes May 3, 2022

View reviewed changes

gouthamve merged commit 2381d7b into main May 3, 2022

gouthamve deleted the otel-context-metadata branch May 3, 2022 18:45

gouthamve mentioned this pull request May 3, 2022

Update Prometheus to not have memory leak. open-telemetry/opentelemetry-collector-contrib#9718

Merged

roidelapluie mentioned this pull request May 6, 2022

Provide target metadata to Appender #10429

Closed

TheSpiritXIII mentioned this pull request May 2, 2023

Upgrade Prometheus to 2.37 GoogleCloudPlatform/prometheus-engine#468

Closed

Send target and metadata cache in context (again) #10636

Send target and metadata cache in context (again) #10636

Uh oh!

Conversation

gouthamve commented Apr 27, 2022

Uh oh!

gouthamve commented Apr 27, 2022

Uh oh!

prombot commented Apr 27, 2022

Uh oh!

roidelapluie commented Apr 27, 2022

Uh oh!

gouthamve commented Apr 28, 2022

Uh oh!

gouthamve commented Apr 28, 2022

Uh oh!

prombot commented Apr 28, 2022

Uh oh!

gouthamve commented May 2, 2022

Uh oh!

roidelapluie left a comment

Choose a reason for hiding this comment

Uh oh!

gouthamve commented May 3, 2022

Uh oh!

Nexucis commented May 3, 2022

Uh oh!

roidelapluie commented May 3, 2022

Uh oh!

roidelapluie left a comment

Choose a reason for hiding this comment

Uh oh!

prombot commented Aug 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants