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

Improve global-cached-lookups metric reporting #13219

Merged

Conversation

zachjsh
Copy link
Contributor

@zachjsh zachjsh commented Oct 12, 2022

It was found that the namespace/cache/heapSizeInBytes metric that tracks the total heap size in bytes of all lookup caches loaded on a service instance was being under reported. We were not accounting for the memory overhead of the String object, which I've found in testing to be ~40 bytes. While this overhead may be java version dependent, it should not vary much, and accounting for this provides a better estimate. Also fixed some logging, and reading bytes from the JDBI result set a little more efficient by saving hash table lookups. Also added some of the lookup metrics to the default statsD emitter metric whitelist.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@@ -245,7 +245,8 @@ static long getByteLengthOfObject(@Nullable Object o)
{
if (null != o) {
if (o.getClass().getName().equals(STRING_CLASS_NAME)) {
return ((String) (o)).length();
// Each String object has ~40 bytes of overhead
return ((long) ((String) (o)).length() * Character.BYTES) + 40;
Copy link
Contributor

@kfaraz kfaraz Oct 13, 2022

Choose a reason for hiding this comment

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

You could also consider doing this instead:

@Override
public long estimateSizeOfValue(String value)
{
// According to https://www.ibm.com/developerworks/java/library/j-codetoheap/index.html
// Total string size = 28B (string metadata) + 16B (char array metadata) + 2B * num letters
return 28 + 16 + (2L * value.length());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! The I think depends on the particular version of java and beyond that the particular distribution of it. In my testing I found 40 bytes to be a good estimate for overhead. Sometimes I find that its a bit less. Will leave as is unless you feel strongly about changing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can proceed with what you have. It's a difference of just 4 bytes (40 vs 44) per String anyway, and as you mention, it does depend on java version.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to use the same calculation and code.
@zachjsh - can you reuse the code? The common code can be placed in StringUtils class.

Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

LGTM

@zachjsh zachjsh merged commit 2f2fe20 into apache:master Oct 13, 2022
@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants