-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[branch-2.9] Group prometheus metrics. #17852
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
[branch-2.9] Group prometheus metrics. #17852
Conversation
|
@tjiuming thanks for fixing this, struggling to find time at the moment |
| out.write(buf.array(), buf.arrayOffset(), buf.readableBytes()); | ||
| } finally { | ||
| //release all the metrics buffers | ||
| metricStreams.releaseAll(); |
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.
@tjiuming I noticed you had removed the exceptionHappens, if we don't have the check, will we release the buf multiple times?
From the master branch:
} catch (Throwable t) {
exceptionHappens = true;
throw t;
} finally {
//release all the metrics buffers
metricStreams.releaseAll();
//if exception happens, release buffer
if (exceptionHappens) {
buf.release();
}
}
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.
@codelipenghui No, this check is for #14453, the feature didn't check-pick to 2.9
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.
@tjiuming will the buf can get a chance to be released multiple times? Since the metricStreams.releaseAll() will also get a chance to release the buf?
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.
@codelipenghui buf and metricStreams only released in the finally scope, and metricStreams.releaseAll() will not release buf
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.
Sorry for missing out on this bug. I wasn't aware that PrometheusMetricsGenerator doesn't look the same in master and in apache-2.9, specifically the generate0, and the performance improvement was backported, so it confused me as well.
The fix looks solid, as this buffer is not returned as it did in generate0 so it should always be released.
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.
@tjiuming Oh, I see. They are different buffers.
Motivation
This PR is fully based on #17618, but the origin PR has a memory leak issue, so it was reverted.
This PR is supposed to fix the memory leak issue.
Why the origin PR has a memory leak issue:
In #17618, PrometheusMetricsGenerator.java:
in the finally scope, call
buf.release()whenexceptionHappens == true. But the initialize value ofexceptionHappensis false, and it never updated to true anywhere. So the memory leak issue happens.It should be a small mistake in the check pick process.
Documentation
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)